aws-controllers-k8s
Use only one of ID or Name while updating Nodegroup
Issue #, if available:
- https://github.com/aws-controllers-k8s/community/issues/2645
Description of changes: Use only one of ID or Name while updating Nodegroup. ID is the priority here.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Yes
Hi @sudohikumar. Thanks for your PR.
I'm waiting for a aws-controllers-k8s 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.
![ack-prow[bot] avatar](https://avatars.githubusercontent.com/in/260409?v=4 "Published by a pub.dev verified publisher"){kind=small}
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: a-hilaly, sudohikumar
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [a-hilaly]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
From one of the other testing, it seems while creation, when we set both id and name of the Launch Template, it errors out with same message InvalidParameterException: Either provide launch template ID or launch template name in the request..
From one of the other testing, it seems while creation, when we set both id and name of the Launch Template, it errors out with same message
InvalidParameterException: Either provide launch template ID or launch template name in the request..
IMO, I think it's fine to surface that error when the user creates a new Nodegroup with both initially set. I'd rather the user see an error than silently ignore their input and take an action they didn't intend. For example, if someone accidentally sets a LaunchTemplate.ID and LaunchTemplate.Name pair that are mismatched and point to different templates. I don't want that error to not get surfaced because we chose to always give one priority over the other when making API calls.
The reason we need to make a priority call when no deltas are detected with either LaunchTemplate.ID and/or LaunchTemplate.Name is that the controller will set a value based on the return of the CreateNodegroup API call so both will be present in the update path. It's safe to make the priority call if there aren't any deltas because we know that the user hasn't made any changes to the manifest that differ from the state in AWS.
Hi @knottnt Thank you for the inputs. It makes sense. I have reverted that change now.
Hi @knottnt Thank you for the inputs. It makes sense. I have reverted that change now.
@sudohikumar Thanks for reverting that. I think this comment still needs to be handled. The reasoning is similar to why the create change was reverted. If the user makes a change we don't want to silently ignore it.
@a-hilaly: The specified target(s) for /test were not found.
The following commands are available to trigger optional jobs:
/test eks-verify-code-gen
Use /test all to run all jobs.
In response to this:
/test eks-code-gen
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.
@sudohikumar: 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 |
|---|---|---|---|---|
| eks-verify-attribution | 026cb7fa86f6693abb60ed74cbc57cef7ba90cc8 | link | false | /test eks-verify-attribution |
| eks-verify-code-gen | 026cb7fa86f6693abb60ed74cbc57cef7ba90cc8 | link | false | /test eks-verify-code-gen |
Full PR test history. Your PR dashboard.
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. I understand the commands that are listed here.