instance-manager icon indicating copy to clipboard operation
instance-manager copied to clipboard

eks-managed nodeLabel field inconsistent with eks and eks-fargate configuration

Open preflightsiren opened this issue 5 years ago • 4 comments

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

preflightsiren avatar Oct 30 '20 22:10 preflightsiren

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.

preflightsiren avatar Oct 30 '20 22:10 preflightsiren

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.

eytan-avisror avatar Nov 02 '20 17:11 eytan-avisror

What about adding labels and merging with nodeLabels and removing nodeLabels is v1alpha2?

preflightsiren avatar Nov 03 '20 12:11 preflightsiren

@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.

eytan-avisror avatar Nov 03 '20 18:11 eytan-avisror