eks-managed nodeLabel field inconsistent with eks and eks-fargate configuration
Is this a BUG REPORT or FEATURE REQUEST?: this is a /nit bug report
What happened:
the eks-managed spec uses a different term for labels. Also the example configuration is invalid because of this inconsistency:
https://github.com/keikoproj/instance-manager/blob/dc47675437a74a74d7f05926aa1c2d42620e12a0/docs/EKS.md
says the field is labels when it is nodeLabels
What you expected to happen:
spec:
eks-managed:
configuration:
labels:
should be valid.
How to reproduce it (as minimally and precisely as possible):
create an InstanceGroup using the example EKS-managed configuration: https://github.com/keikoproj/instance-manager/blob/dc47675437a74a74d7f05926aa1c2d42620e12a0/docs/EKS.md
and labels specified will not be assigned.
Anything else we need to know?: eks Labels: https://github.com/keikoproj/instance-manager/blob/master/api/v1alpha1/instancegroup_types.go#L192 eks-managed nodeLabels: https://github.com/keikoproj/instance-manager/blob/master/api/v1alpha1/instancegroup_types.go#L247 eks-fargate Labels: https://github.com/keikoproj/instance-manager/blob/master/api/v1alpha1/instancegroup_types.go#L260
I'm happy to fix this up after LaunchTemplates get implemented - should just be a case of adding the new field name and merging them together to ensure backwards compatibility.
I think we can add this as a suggestion in #113 This is an API change and using API Versions is the best way to have backwards compatible changes to API. I agree that this will make the API a bit more consistent (all provisioners using similar terms), but since this is not causing a problem we should probably only make this change when we start working on v1alpha2.
What about adding labels and merging with nodeLabels and removing nodeLabels is v1alpha2?
@preflightsiren That would definitely be possible, but kinda defeats the purpose of having APIVersions in Kubernetes :) If this is affecting you in some bad way (let me know if it does) I am happy to accept a change, but if it's just for 'correctness' of the API with other APIs (eks, fargate, etc) - which you are probably correct about it being nicer if all were using same term, then we can probably do this down the road with the next API version for a cleaner transition.
I updated #113 to include this change.