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

Remove etcd_version #7992

Merged

Conversation

vrutkovs
Copy link
Member

etcd version can no longer be specified in etcd_version.

This parameter never worked properly, as for various platforms a minimum version was required. Instead of some outdated version the latest etcd should always be used

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1538989

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 17, 2018
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2018
@vrutkovs
Copy link
Member Author

/test gcp

Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about this. It looks like the parameter would work in some cases.

@sdodson
Copy link
Member

sdodson commented Apr 17, 2018

We don't exert any control over etcd version other than when we upgrade it we do so in a step wise manner. This definitely didn't work during upgrades and if we were to make it work during upgrades we'd have to add a lot of conditional logic to ensure that it's both stepwise and how to handle the situation where we'd have to downgrade in order to get the version requested.

Therefore I think we should just remove this.

@michaelgugino
Copy link
Contributor

We don't exert any control over etcd version other than when we upgrade it we do so in a step wise manner. This definitely didn't work during upgrades and if we were to make it work during upgrades we'd have to add a lot of conditional logic to ensure that it's both stepwise and how to handle the situation where we'd have to downgrade in order to get the version requested.

I do think we need to refactor how etcd upgrades take place (I believe origin is currently broken in this regard).

In the mean time, I'm okay with removing it if that's desired.

Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2018
Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

/lgtm

@sdodson
Copy link
Member

sdodson commented Apr 17, 2018

/test gcp

@openshift-merge-robot openshift-merge-robot merged commit b1c9599 into openshift:master Apr 18, 2018
@vrutkovs
Copy link
Member Author

/cherrypick release-3.9

@openshift-cherrypick-robot

@vrutkovs: #7992 failed to apply on top of branch "release-3.9":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	inventory/hosts.example
M	roles/etcd/tasks/certificates/fetch_server_certificates_from_ca.yml
A	roles/etcd/tasks/rpm.yml
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): roles/etcd/tasks/rpm.yml deleted in HEAD and modified in Remove etcd_version. Version Remove etcd_version of roles/etcd/tasks/rpm.yml left in tree.
Auto-merging roles/etcd/tasks/certificates/fetch_server_certificates_from_ca.yml
Auto-merging inventory/hosts.example
Patch failed at 0001 Remove etcd_version

In response to this:

/cherrypick release-3.9

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.

@vrutkovs vrutkovs deleted the remove-etcd-version branch January 5, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants