Skip to content

✨ Refactor to consolidate into LaunchTemplateNeedsUpdate #5511

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Refactor to consolidate into LaunchTemplateNeedsUpdate #5511

wants to merge 1 commit into from

Conversation

fiunchinho
Copy link
Contributor

What type of PR is this?

/kind design

What this PR does / why we need it:

We have a function called LaunchTemplateNeedsUpdate that checks different LaunchTemplate fields in the existing Launch Template and the incoming one, to decide whether or not we need a new version of the Launch Template. Outside of this function, we are also checking other fields, making code a bit confusing, and requiring some extra variables that we need to always check together to finally decide if a new Launch Template version is needed.

With these changes, we move as much code as possible into LaunchTemplateNeedsUpdate so that everything is in the same place.

Special notes for your reviewer:

I also renamed some variables to make the code easier to read.

Checklist:

  • squashed commits
  • includes documentation
  • includes emoji in title
  • adds unit tests
  • adds or updates e2e tests

Release note:

Refactor to consolidate into LaunchTemplateNeedsUpdate

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/design Categorizes issue or PR as related to design. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nrb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from AndiDog and faiq May 28, 2025 16:39
@nrb
Copy link
Contributor

nrb commented Jun 1, 2025

Skimmed this really quickly, it looks good to me modulo the linting failures. I assume you're waiting on #5506 #5507 #5508 for taking this out of draft?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2025
@fiunchinho
Copy link
Contributor Author

Skimmed this really quickly, it looks good to me modulo the linting failures. I assume you're waiting on #5506 #5507 #5508 for taking this out of draft?

Yes, correct. Once all of those are merged, I'll finish this one.

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from e05bf8e to da6c2fe Compare June 5, 2025 16:55
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2025
@fiunchinho fiunchinho marked this pull request as ready for review June 5, 2025 16:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2025
@k8s-ci-robot k8s-ci-robot requested a review from luthermonson June 5, 2025 16:55
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from da6c2fe to df5b167 Compare June 5, 2025 17:07
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from df5b167 to ca499fa Compare June 6, 2025 17:33
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@fiunchinho
Copy link
Contributor Author

/retest

1 similar comment
@fiunchinho
Copy link
Contributor Author

/retest

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from ca499fa to 2638a92 Compare June 7, 2025 11:48
@fiunchinho
Copy link
Contributor Author

/retest

@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

fiunchinho
fiunchinho commented Jun 7, 2025
View reviewed changes
exp/controllers/awsmachinepool_controller_test.go
@@ -749,60 +749,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
g.Expect(err).To(Succeed())
})

t.Run("launch template and ASG exist and only AMI ID changed", func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now tested inside the LaunchTemplateNeedsUpdate function.

@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

1 similar comment
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@richardcase
Copy link
Member

@fiunchinho - if you rebase on main then hopefully the e2e will pass. Ping me if they don't.

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from 2638a92 to 394743e Compare June 9, 2025 08:11
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

1 similar comment
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@fiunchinho
Copy link
Contributor Author

@richardcase It looks like something is wrong with the webhooks, I can't seem to be able to trigger the e2e tests.

@richardcase
Copy link
Member

Looks like the multi-tenancy test is still failing. I will investigate.

@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@richardcase
Copy link
Member

/test ?

@k8s-ci-robot
Copy link
Contributor

@richardcase: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-aws-build

/test pull-cluster-api-provider-aws-build-docker

/test pull-cluster-api-provider-aws-test

/test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main

/test pull-cluster-api-provider-aws-e2e

/test pull-cluster-api-provider-aws-e2e-blocking

/test pull-cluster-api-provider-aws-e2e-clusterclass

/test pull-cluster-api-provider-aws-e2e-conformance

/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts

/test pull-cluster-api-provider-aws-e2e-eks

/test pull-cluster-api-provider-aws-e2e-eks-gc

/test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main

pull-cluster-api-provider-aws-build

pull-cluster-api-provider-aws-build-docker

pull-cluster-api-provider-aws-test

pull-cluster-api-provider-aws-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@richardcase
Copy link
Member

/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e

@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@richardcase
Copy link
Member

Re-running the e2e as we hit the invalid security context error

/test pull-cluster-api-provider-aws-e2e

@fiunchinho
Copy link
Contributor Author

/retest

@fiunchinho fiunchinho force-pushed the consolidate-launchtemplate-diff branch from 394743e to b655bef Compare June 11, 2025 07:31
@fiunchinho
Copy link
Contributor Author

/test pull-cluster-api-provider-aws-e2e

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

@fiunchinho: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-aws-e2e b655bef link false /test pull-cluster-api-provider-aws-e2e
pull-cluster-api-provider-aws-e2e-blocking b655bef link true /test pull-cluster-api-provider-aws-e2e-blocking

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndiDog AndiDog Awaiting requested review from AndiDog

@faiq faiq Awaiting requested review from faiq

@luthermonson luthermonson Awaiting requested review from luthermonson

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fiunchinho @k8s-ci-robot @nrb @richardcase