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.

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

AWSManagedMachinePool - Launch Template support #3094

Merged
merged 10 commits into from
Aug 31, 2022
Merged

AWSManagedMachinePool - Launch Template support #3094

merged 10 commits into from
Aug 31, 2022

Conversation

richardchen331
Copy link
Contributor

@richardchen331 richardchen331 commented Jan 22, 2022 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add launch template support to AWSManagedMachinePool.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2055

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

Add launch template support to `AWSManagedMachinePool`.

@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. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 22, 2022
@k8s-ci-robot
Copy link
Contributor

@richardchen331: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

Hi @richardchen331. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2022
@richardcase
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 22, 2022
@richardchen331 richardchen331 marked this pull request as ready for review January 23, 2022 00:39
@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 Jan 23, 2022
@richardchen331
Copy link
Contributor Author

/retest

@richardchen331
Copy link
Contributor Author

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

@sedefsavas
Copy link
Contributor

@richardchen331 @richardcase is this implementation in line with #3024?

@richardcase
Copy link
Member

@richardchen331 @richardcase is this implementation in line with #3024?

@sedefsavas - i need to update the proposal based on a chat that i had with @richardchen331. We decided on a way forward to unblock development. If after the proposal has been updated and changes are needed we can go back and refactor. I will check the proposal updated by EOD Wednesday.

@richardcase richardcase mentioned this pull request Jan 31, 2022
Closed
2 tasks
@richardcase richardcase mentioned this pull request Feb 7, 2022
Open
@sedefsavas
Copy link
Contributor

Given that this is an important feature and changes the way we interact with AWSManagedMachinePool, we would want to have this in e2e tests and there is already a proposal out but would be good to update existing docs.

@sedefsavas sedefsavas mentioned this pull request Feb 10, 2022
Closed
@richardcase
Copy link
Member

Given that this is an important feature and changes the way we interact with AWSManagedMachinePool, we would want to have this in e2e tests and there is already a proposal out but would be good to update existing docs.

Agree on adding this to the e2e tests for eks. This is covered in the updated proposal. @richardchen331 i can guide you through this if you want?

@richardchen331
Copy link
Contributor Author

Given that this is an important feature and changes the way we interact with AWSManagedMachinePool, we would want to have this in e2e tests and there is already a proposal out but would be good to update existing docs.

Agree on adding this to the e2e tests for eks. This is covered in the updated proposal. @richardchen331 i can guide you through this if you want?

@richardcase Agree that we should have e2e tests for it. It would be great if you could guide me on how to add e2e tests for eks :)

@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

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

3 similar comments
@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

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

@richardchen331
Copy link
Contributor Author

@pydctw Thanks for all the feedbacks! I've updated the PR based on your comments. Could you take another look? Thanks!

@pydctw
Copy link
Contributor

pydctw commented Aug 15, 2022

Thanks for addressing all the comments, @richardchen331. It looks good to me.

Two remaining items that are related regarding the usage of launchtemplate for AWSManagedMachinePool. Let's wait for @richardcase's opinion on this.

FYI, we are trying to include the PR in the next release.

@richardchen331
Copy link
Contributor Author

Thanks for addressing all the comments, @richardchen331. It looks good to me.

Two remaining items that are related regarding the usage of launchtemplate for AWSManagedMachinePool. Let's wait for @richardcase's opinion on this.

FYI, we are trying to include the PR in the next release.

@richardcase @pydctw just want to follow up and see if we have made any decision about these two comments? Thanks!

@pydctw
Copy link
Contributor

pydctw commented Aug 25, 2022

@richardcase @pydctw just want to follow up and see if we have made any decision about these two comments? Thanks!

Thanks for the reminder., @richardchen331. I planned to update the PR after the office hrs but forgot.

As there was no strong opinion whether to 1) Deprecate the existing fields and only support LaunchTemplate or 2) Support both existing and new way (LaunchTemplate), I propose we proceed as following.

  • Merge the PR as is - it looks good and all test passed.
  • Discuss two options in the proposal.
  • Depending on the decision, create a new PR to mark those fields as deprecated if we decided on option 1, otherwise do nothing.

@pydctw
Copy link
Contributor

pydctw commented Aug 25, 2022

/lgtm

cc @richardcase for an approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2022
@pydctw pydctw mentioned this pull request Aug 25, 2022
Merged
4 tasks
@sedefsavas
Copy link
Contributor

/lgtm

@richardcase
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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

The pull request process is described 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0402f41 into kubernetes-sigs:main Aug 31, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.x milestone Aug 31, 2022
@richardcase richardcase mentioned this pull request Sep 2, 2022
Closed
sadysnaat pushed a commit to spectrocloud/cluster-api-provider-aws that referenced this pull request Nov 16, 2022
@k8s-ci-robot @sadysnaat
Merge pull request kubernetes-sigs#3094 from richardchen331/launch_te…
61ff6ae 
…mplate_support

AWSManagedMachinePool - Launch Template support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankitasw Ankitasw Awaiting requested review from Ankitasw

@sedefsavas sedefsavas Awaiting requested review from sedefsavas

@richardcase richardcase Awaiting requested review from richardcase

@pydctw pydctw Awaiting requested review from pydctw

Assignees

@pydctw pydctw

@sedefsavas sedefsavas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWSManagedMachinePool - Launch Template support
5 participants
@richardchen331 @k8s-ci-robot @richardcase @sedefsavas @pydctw