Skip to content

Conversation

@estroz
Copy link
Member

@estroz estroz commented Feb 2, 2021

Description of the change:

  • internal/olm/operator: validate BundleAddMode before running a bundle image

Motivation for the change: values should be validated before the heavy lifting of these commands occurs (pulling/unpacking a bundle)

/kind bug

Signed-off-by: Eric Stroczynski [email protected]

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 2, 2021
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

Validation part looks good to me, but I'm thinking about why we need to have bundle add mode as an optional hidden flag in the case of run bundle-upgrade, in the first place.

For run bundle, we explicitly said we need it. But for upgrade, is there is a good reason to have it as an optional hidden flag?

@estroz
Copy link
Member Author

estroz commented Feb 2, 2021

@rashmigottipati good question. I argue that bundle add mode is more important for bundle-upgrade than bundle because you’re testing the functionality of bundle plus the upgrade path explicitly, which may differ/break depending on which mode you use.

Copy link
Member

@jmrodri jmrodri 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 Feb 2, 2021
@estroz estroz force-pushed the bugfix/validate-add-mode branch from 0a90f9e to 7041102 Compare February 2, 2021 18:10
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021
Copy link
Member

@rashmigottipati rashmigottipati 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 Feb 2, 2021
@estroz estroz merged commit 172360a into operator-framework:master Feb 2, 2021
@estroz estroz deleted the bugfix/validate-add-mode branch February 2, 2021 18:35
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 4, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 4, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…erator-framework#4462)

Signed-off-by: Eric Stroczynski <[email protected]>
Signed-off-by: Rein van 't Veer <[email protected]>
Signed-off-by: reinvantveer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants