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

Add references to launch templates from EKS node groups #853

Conversation

oblogic7
Copy link

Description of your changes

Fixes #851

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Guidance needed.

@Upbound-CLA
Copy link

Upbound-CLA commented Aug 29, 2023

CLA assistant check
All committers have signed the CLA.

@oblogic7
Copy link
Author

I have read the crossplane and upjet contribution guides. I'm still not certain of the most efficient path for testing changes to a provider. Specifically, is there tooling or commands available that offer an efficient path for standing up a local dev environment for testing provider changes?

@turkenf
Copy link
Collaborator

turkenf commented Aug 29, 2023

Hi @oblogic7,

Thank you for your contribution. Here is a guide on how you can test it, are you aware of it? You must be a contributor to trigger the automatic test specified in the guide. If the resource or feature you add is uptestable, we can leave this comment for you in your first PR.

Let's talk about this PR, here we add reference configuration for launch_template.id to resource NodeGroup.eks. The launch_template parameter does not exist in our current example because it is an optional parameter. Here we can try a few ways to test:

  • We can test manually using the selector we added in our local environment.
  • As in this PR, we can create a new example that uses the selector we added and contains the dependent resources and can test it automatically with uptest or manually.(e.g. examples/eks/nodegroup-with-launchtemplate.yaml)

In both cases we should test examples/eks/nodegroup.yaml to make sure that the configuration we are adding does not break the existing resource.

@turkenf
Copy link
Collaborator

turkenf commented Aug 29, 2023

/test-examples="examples/eks/nodegroup.yaml"

1 similar comment
@turkenf
Copy link
Collaborator

turkenf commented Aug 29, 2023

/test-examples="examples/eks/nodegroup.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Aug 31, 2023

@oblogic7 uptest run failed due to the dependent resources did not exist in the example manifest, we should test manually.

apis/eks/v1beta1/zz_nodegroup_types.go Show resolved Hide resolved
Comment on lines +56 to +58
Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.LaunchTemplate",
RefFieldName: "LaunchTemplateIDRefs",
SelectorFieldName: "LaunchTemplateIDSelector",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.LaunchTemplate",
RefFieldName: "LaunchTemplateIDRefs",
SelectorFieldName: "LaunchTemplateIDSelector",
Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.LaunchTemplate",
TerraformName: "aws_kinesis_stream",
Extractor: common.PathTerraformIDExtractor,

The better way to use TerraformName even if it is difficult, in order not to be affected by any group changes.
And we'd better not use RefFieldName and SelectorFieldName unless you have a specific reason to come up with it that way. It is used in the configuration you added as follows:

spec:
  launchTemplate:
    launchTemplateIdSelector:
      matchLabels:
        testing.upbound.io/example-name: ....

But as recommended:

spec:
  launchTemplate:
    IDSelector:
      matchLabels:
        testing.upbound.io/example-name: ....

What do you think?

Comment on lines +60 to +64
r.References["launch_template.name"] = config.Reference{
Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.LaunchTemplate",
RefFieldName: "LaunchTemplateNameRefs",
SelectorFieldName: "LaunchTemplateNameSelector",
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may not add this as name and id cannot be used together.

@turkenf
Copy link
Collaborator

turkenf commented Sep 27, 2023

@oblogic7, if you are considering taking this PR forward, please check the comments.

@turkenf
Copy link
Collaborator

turkenf commented Oct 11, 2023

I am closing this now if you want to take forward feel free to reopen it.

@turkenf turkenf closed this Oct 11, 2023
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.

Need to enable launch template ref from EKS node groups
4 participants