-
Notifications
You must be signed in to change notification settings - Fork 199
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
Fix/cluster tags #662
Fix/cluster tags #662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elamaran11 awesome change, only a couple of minor inline comments.
A bigger question is: it is odd that the change affects only Fargate and Generic cluster providers but not MngClusterProvider or AsgClusterProvider.
I realize that MngClusterProviderProps
and such already have tags attribute but that was meant for the nodegroup tags, not the cluster.
If a customer passes tags with either MngClusterProvider or AsgClusterProvider, both node group and cluster will be tagged. This may be counter-intuitive.
Resolution:
- Rename tags clearly and use
clusterTags
vsnodeGroupTags
to avoid confusion across the board.
OR
- Use tags consistently across the board, however for Mng and Asg cases document that provided tags will apply both to the nodegroup and the cluster.
@@ -2,6 +2,7 @@ import { Construct } from "constructs"; | |||
import * as ec2 from "aws-cdk-lib/aws-ec2"; | |||
import * as eks from "aws-cdk-lib/aws-eks"; | |||
import { defaultOptions, GenericClusterProvider } from './generic-cluster-provider'; | |||
import { Tags } from "aws-cdk-lib"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github actions warning - make lint please
@@ -1,4 +1,4 @@ | |||
# FluxCD Add-on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, approving. merge pending e2e
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests passed
Issue #, if available:
#631
Description of changes:
Tags for GenericClusterProvider and FargateClusterProvider.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.