Add aws-node-termination-handler, fix test values filenames #954
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Andreas Sommer <[email protected]>
AndiDog
added
the
skip/ci
|
Manual test worked fine |
fiunchinho
reviewed
Dec 9, 2024
| @@ -0,0 +1,61 @@ | |||
| {{/* Default Helm values for the app */}} | |||
| {{/* See schema for the appropriate app version here https://github.com/giantswarm/aws-ebs-csi-driver-app/blob/master/helm/aws-ebs-csi-driver-app/values.schema.json */}} | |||
There was a problem hiding this comment.
Suggested change
| {{/* See schema for the appropriate app version here https://github.com/giantswarm/aws-ebs-csi-driver-app/blob/master/helm/aws-ebs-csi-driver-app/values.schema.json */}} | |
| {{/* See schema for the appropriate app version here https://github.com/giantswarm/aws-node-termination-handler-app/blob/main/helm/aws-node-termination-handler/values.schema.json */}} |
There was a problem hiding this comment.
Done but using https://github.com/giantswarm/aws-nth-bundle
fiunchinho
reviewed
Dec 9, 2024
| cluster-apps-operator.giantswarm.io/watching: "" | ||
| {{- include "labels.common" . | nindent 4 }} | ||
| spec: | ||
| suspend: false # It can be unsuspended by the post-install/post-upgrade hook. Useful if we need to populate some fields later on. |
There was a problem hiding this comment.
GROUP: helm.toolkit.fluxcd.io
KIND: HelmRelease
VERSION: v2beta1
FIELD: suspend <boolean>
DESCRIPTION:
Suspend tells the controller to suspend reconciliation for this HelmRelease,
it does not apply to already started reconciliations. Defaults to false.
Being false the default, I'd remove it from here to avoid adding unnecessary noise.
fiunchinho
reviewed
Dec 9, 2024
helm/cluster-aws/values.schema.json
Outdated
| "awsNodeTerminationHandler": { | ||
| "$ref": "#/$defs/helmRelease", | ||
| "type": "object", | ||
| "title": "AWS EBS CSI driver", |
There was a problem hiding this comment.
Suggested change
| "title": "AWS EBS CSI driver", | |
| "title": "AWS Node Termination Handler", |
There was a problem hiding this comment.
Don't forget this one. Other than that, LGTM
There was a problem hiding this comment.
Fixed, but it's not even used in README.md 😆
fiunchinho
reviewed
Dec 9, 2024
helm/cluster-aws/values.schema.json
Outdated
| @@ -1665,6 +1671,11 @@ | |||
| "minLength": 1, | |||
| "pattern": "^[-a-zA-Z0-9_\\.]{1,63}$" | |||
| }, | |||
| "awsPartition": { | |||
There was a problem hiding this comment.
If this is used for local rendering, I was wondering if it would make more sense to put it under the internal field. It's tricky, because internal may end up being a "everything fits in here" bucket for different fields, but I guess internal was added for fields that were supposed to be used by us, not the customer, like this one.
There was a problem hiding this comment.
Since cluster-aws has its own internal.*, I can easily add it there. It won't clash with cluster's cluster.internal.* values. Done, please check.
|
There were differences in the rendered Helm template, please check! Output |
fiunchinho
approved these changes
Dec 10, 2024
AndiDog
added a commit
that referenced
this pull request
Dec 10, 2024
Co-authored-by: paurosello <[email protected]>
This was referenced Dec 10, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Towards https://github.com/giantswarm/giantswarm/issues/31843. Backport of #945.
I'm manually testing and then merging this, given that E2E tests currently don't work if a new application gets added (see original PR).
Checklist
Trigger E2E tests
E2E test will fail – same as in original PR.