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

Require extra flag when updating cluster with downgraded kops version #9362

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Require extra flag when updating cluster with downgraded kops version #9362

merged 1 commit into from
Jul 17, 2020

Conversation

johngmyers
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2020
hakman
hakman reviewed Jun 15, 2020
View reviewed changes
pkg/apis/kops/registry/registry.go Outdated
@@ -29,6 +29,8 @@ const (
PathCluster = "config"
// Path for completed cluster spec in the state store
PathClusterCompleted = "cluster.spec"
// PathKopsVersion is the path for the version of kops last used to apply the cluster.
PathKopsVersionApplied = "kops-version-applied.txt"
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be closer to cluster.spec, master-a, config that are in the bucket root.

Suggested change
PathKopsVersionApplied = "kops-version-applied.txt"
PathKopsVersionApplied = "kops.version"

Copy link
Member Author

Choose a reason for hiding this comment

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

Having the well-known .txt extension helps with troubleshooting.

hakman
hakman requested changes Jun 15, 2020
View reviewed changes
upup/pkg/fi/cloudup/apply_cluster.go Outdated
if err == nil {
version, err := semver.Parse(strings.TrimSpace(string(kopsVersionApplied)))
if err != nil {
return fmt.Errorf("error parsing last kops version applied: %v", err)
Copy link
Member

@hakman hakman Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("error parsing last kops version applied: %v", err)
return fmt.Errorf("error parsing last kops version last used to update: %v", err)

Copy link
Member Author

@johngmyers johngmyers Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's not the last version used. One can use a downrev or uprev kops to edit the cluster or ig specs; it only matters which version of kops last did an apply_cluster without dryrun.

Copy link
Member

Choose a reason for hiding this comment

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

I understand this, but it's more from the developer's point of view. "Used" is more user oriented.
In any case, even using "last kops version used to apply changes" would be a bit better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps "updated", since the CLI command is kops update cluster --yes. The apply_cluster.go is mixing terminology a bit.

Copy link
Member

Choose a reason for hiding this comment

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

That would be even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Went with the "updated" terminology.

Copy link
Member

Choose a reason for hiding this comment

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

Probably you want to update this message also.

pkg/apis/kops/registry/registry.go Outdated
Comment on lines 32 to 33
// PathKopsVersion is the path for the version of kops last used to apply the cluster.
PathKopsVersionApplied = "kops-version-applied.txt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// PathKopsVersion is the path for the version of kops last used to apply the cluster.
PathKopsVersionApplied = "kops-version-applied.txt"
// PathKopsVersionUsed is the path for the version of kops last used to apply the cluster.
PathKopsVersionUsed = "kops.version"

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, it's not the last kops version used.

hakman
hakman reviewed Jun 16, 2020
View reviewed changes
pkg/apis/kops/registry/registry.go Outdated
@@ -29,6 +29,8 @@ const (
PathCluster = "config"
// Path for completed cluster spec in the state store
PathClusterCompleted = "cluster.spec"
// PathKopsVersionUpdated is the path for the version of kops last used to apply the cluster.
PathKopsVersionUpdated = "kops-version-updated.txt"
Copy link
Member

Choose a reason for hiding this comment

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

Should be enough without updated.

Suggested change
PathKopsVersionUpdated = "kops-version-updated.txt"
PathKopsVersionUpdated = "kops-version.txt"

@hakman
Copy link
Member

hakman commented Jun 16, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 16, 2020
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 22, 2020
@johngmyers
Copy link
Member Author

/retest

1 similar comment
@johngmyers
Copy link
Member Author

/retest

@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 28, 2020
@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 28, 2020
@olemarkus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2020
@rifelpet
Copy link
Member

Agreed during office hours that this is good to merge as-is
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, rifelpet

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 Jul 17, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020
@hakman
Copy link
Member

hakman commented Jul 17, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7b50468 into kubernetes:master Jul 17, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 17, 2020
@johngmyers johngmyers deleted the avoid-downgrade branch July 17, 2020 19:23
@mikejoh mikejoh mentioned this pull request Sep 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hakman hakman hakman requested changes

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

Assignees

@olemarkus olemarkus

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/documentation 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
5 participants
@johngmyers @hakman @olemarkus @rifelpet @k8s-ci-robot