Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node-local-dns #1

Merged
merged 14 commits into from
Aug 1, 2023
Merged

node-local-dns #1

merged 14 commits into from
Aug 1, 2023

Conversation

MonolithProjects
Copy link
Contributor

Description

Initial addon setup

Type of change

  • A bug fix (PR prefix fix)
  • A new feature (PR prefix feat)
  • A code change that neither fixes a bug nor adds a feature (PR prefix refactor)
  • Adding missing tests or correcting existing tests (PR prefix test)
  • Changes that do not affect the meaning of the code like white-spaces, formatting, missing semi-colons, etc. (PR prefix style)
  • Changes to our CI configuration files and scripts (PR prefix ci)
  • Documentation only changes (PR prefix docs)

How Has This Been Tested?

@MonolithProjects MonolithProjects self-assigned this Jun 23, 2023
variables.tf Outdated Show resolved Hide resolved
values.tf Outdated
Comment on lines 97 to 100
"serviceAccount" : {
"create" : true,
"annotations" : {},
"name" : ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix alignment with module input variables:

  • var.service_account_create
  • var.service_account_name
  • IRSA machinery annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

values.tf Outdated Show resolved Hide resolved
values.tf Show resolved Hide resolved
@MonolithProjects MonolithProjects changed the title WIP: node-local-dns node-local-dns Jul 17, 2023
values.tf Show resolved Hide resolved
@@ -1,9 +1,132 @@
locals {
values_default = yamlencode({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the defaults are also defaults in the Helm chart that is used under the hood (https://github.com/lablabs/k8s-nodelocaldns-helm/blob/master/charts/node-local-dns/values.yaml). I think we can remove the same defaults and configure only AWS EKS specific values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree but same as the irsa, i will check it in second iteration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, please, just raise a Jira ticket for this work so we got it tracked.

@MonolithProjects
Copy link
Contributor Author

@dojci now the PR is ready to be reviewed

values.tf Show resolved Hide resolved
values.tf Outdated
}
})

release_name_suffixed = "${var.helm_release_name}-${one(random_pet.release_name_suffix[*].id)}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release_name_suffixed -> helm_release_name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@dojci dojci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MonolithProjects MonolithProjects merged commit 1a6a674 into main Aug 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants