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

internal/olm: get installed version from cluster instead of required CLI argument #1634

Merged
merged 12 commits into from
Aug 15, 2019

Conversation

estroz
Copy link
Member

@estroz estroz commented Jul 2, 2019

Description of the change: get the version of OLM installed in a cluster without passing --version=<version> to olm status.

Motivation for the change: a user may not know which OLM version was installed in a cluster.

@joelanford we might be able to use Client.GetInstalledVersion() in Uninstall() too.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2019
@joelanford
Copy link
Member

@estroz Makes sense to me to use it for uninstall.

--version is only used with Install.

internal/olm/manager.go: use Client.GetInstalledVersion() in
Manager.Uninstall() instead of requiring version.
@estroz estroz force-pushed the olm-get-installed-version branch 2 times, most recently from 5affa6a to 4fdf055 Compare July 2, 2019 17:20
@estroz estroz changed the title [WIP] internal/olm: get installed version using Client.GetInstalledVersion() in Manager.Status() internal/olm: get installed version using Client.GetInstalledVersion() in Manager.Status() Jul 2, 2019
@openshift-ci-robot openshift-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 Jul 2, 2019
@estroz estroz requested review from joelanford and lilic July 2, 2019 17:23
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

After addressing comments and if OLM agrees that this is a reasonable way to determine the OLM version.

cmd/operator-sdk/alpha/olm/install.go Outdated Show resolved Hide resolved
hack/tests/alpha-olm-subcommands.sh Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019
@estroz
Copy link
Member Author

estroz commented Jul 2, 2019

@joelanford indeed they do: operator-framework/operator-lifecycle-manager#935.

@estroz estroz force-pushed the olm-get-installed-version branch from 4fdf055 to 3db2094 Compare July 2, 2019 21:02
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019
Co-Authored-By: Joe Lanford <joe.lanford@gmail.com>
@lilic
Copy link
Member

lilic commented Jul 5, 2019

/test e2e-aws-ansible

@estroz
Copy link
Member Author

estroz commented Jul 8, 2019

/cc @hasbro17 @lilic @AlexNPavel

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2019
@estroz
Copy link
Member Author

estroz commented Jul 30, 2019

/cc @jmrodri @fabianvf @theishshah

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2019
@estroz estroz requested a review from joelanford August 12, 2019 18:34
@estroz estroz changed the title internal/olm: get installed version using Client.GetInstalledVersion() in Manager.Status() internal/olm: get installed version from cluster instead of required CLI argument Aug 13, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

One nit. Otherwise LGTM

internal/olm/manager.go Outdated Show resolved Hide resolved
@joelanford
Copy link
Member

@estroz Can you also update doc/sdk-cli-reference.md and the CHANGELOG to reflect these changes?

@estroz estroz merged commit 24f9e36 into operator-framework:master Aug 15, 2019
@estroz estroz deleted the olm-get-installed-version branch August 15, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants