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

LaunchTemplate Version Name Fix .. #7118

Closed
wants to merge 1 commit into from

Conversation

gambol99
Copy link
Contributor

@gambol99 gambol99 commented Jun 7, 2019

The commit eca2b0a added the version to the name of the which created a loop on NeedUpdate, as the empty version will never be equal to the original name.

  • Revert "User version number as well as name when checking LaunchTemplates

This reverts commit eca2b0a.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 7, 2019
@gambol99
Copy link
Contributor Author

gambol99 commented Jun 7, 2019

/assign @KashifSaadat

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99

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 Jun 7, 2019
@gambol99
Copy link
Contributor Author

gambol99 commented Jun 7, 2019

fixes #6796

@granular-ryanbonham
Copy link
Contributor

Reverting this will cause us to reintroduce #6751 where launchtemplate are not rolled correctly. I am looking over the code more to see why we are seeing different behavior.

@gambol99
Copy link
Contributor Author

gambol99 commented Jun 7, 2019

So having played about with the a terraform target things appears to be working as expected, updates to the launch_template and autoscaling_group are reflected. The main difference is the way terraform is handling the launch_template; in api target we aren't using versions, but instead follow the same pattern as launch_configurations and creating a new LG on change; terraform appears to create a new version and thanks to the ASG using latest_version the configuration is referenced .. So just to be on the same page, is the issue in #6751 purely that the nodes which need updating are not showing up in a kops update cluster? .. @granular-ryanbonham i.e. changes are reflected by terraform in the LT and ASG but when running the command the nodes which should show up as NeedUpdate do not? ...

@KashifSaadat
Copy link
Contributor

Hey @granular-ryanbonham, any luck further debugging this?

jaredallard added a commit to getoutreach/kops that referenced this pull request Aug 14, 2019
@rifelpet
Copy link
Member

/test pull-kops-verify-gomod

@rifelpet
Copy link
Member

is this no longer needed now that #7445 is merged?

@k8s-ci-robot
Copy link
Contributor

@gambol99: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kops-verify-generated 2188281 link /test pull-kops-verify-generated
pull-kops-verify-staticcheck 2188281 link /test pull-kops-verify-staticcheck

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/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2019
@rifelpet
Copy link
Member

I'm fairly certain this is no longer needed but if it is, feel free to reopen

/close

@k8s-ci-robot
Copy link
Contributor

@rifelpet: Closed this PR.

In response to this:

I'm fairly certain this is no longer needed but if it is, feel free to reopen

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants