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

Adding ability to deprecate bundles #397

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Adding ability to deprecate bundles #397

merged 1 commit into from
Jul 30, 2020
+844 −8

Conversation

gallettilance
Copy link
Member

Adding support marking versions as deprecated and truncating the update
graph of a given package. Deprecated versions will not be installable.
If a deprecated version is installed prior to upgrade appropriate alerts
will be fired on cluster.

Description of the change:

Described in this enhancement

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

Sorry, something went wrong.

@gallettilance gallettilance mentioned this pull request Jul 22, 2020
Closed
5 tasks
shawn-hurley
shawn-hurley requested changes Jul 22, 2020
View reviewed changes
Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

  • We need tests
  • Add an example string for the command
  • We need to enhance add to undo the deprecation (add back deprecated bundle mark it's replaced bundle as deprecated)

@kevinrizza
Copy link
Member

@shawn-hurley

We need to enhance add to undo the deprecation (add back deprecated bundle mark it's replaced bundle as deprecated)

Is that part necessary? I keep hearing about these requests from the pipelines that they need to have "undo" commands. But can't they just... point to a version of the index that didn't remove it yet?

@gallettilance
Copy link
Member Author

In the enhancement we punted on that command https://github.com/operator-framework/enhancements/blob/master/enhancements/deprecated-bundles.md#story-2 in favor of using images as @kevinrizza suggested

ecordell
ecordell reviewed Jul 22, 2020
View reviewed changes
dinhxuanvu
dinhxuanvu reviewed Jul 23, 2020
View reviewed changes
@dinhxuanvu dinhxuanvu added the area/dependency Issues or PRs related to dependency changes label Jul 23, 2020
@gallettilance gallettilance force-pushed the deprecate-bundles branch 6 times, most recently from 3349788 to f3cd926 Compare July 27, 2020 12:49
shawn-hurley
shawn-hurley approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

looks good to me

kevinrizza
kevinrizza requested changes Jul 27, 2020
View reviewed changes
benluddy
benluddy reviewed Jul 27, 2020
View reviewed changes
@kevinrizza
Copy link
Member

/retest

1 similar comment
@gallettilance
Copy link
Member Author

/retest

awgreene
awgreene requested changes Jul 27, 2020
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

A few open questions. Nice work @gallettilance

@kevinrizza
Copy link
Member

/lgtm

@gallettilance
Copy link
Member Author

/retest

1 similar comment
@gallettilance
Copy link
Member Author

/retest

@gallettilance
Copy link
Member Author

/retest

@kevinrizza
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2020
ecordell
ecordell reviewed Jul 28, 2020
View reviewed changes
pkg/registry/populator_test.go
},
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

this is looking pretty good, but doesn't quite convince me that there are no bugs - at the very least I think this should verify the full end state of the graph, not just the bundles/packages/channels.

I also want to make sure that this covers:

  • bundle is in two channels
  • bundle is in two packages
  • bundle is skipped by another bundle
  • bundle has children that are skipped that get removed

(it might already cover these implicitly, just need to check)

@dinhxuanvu
Copy link
Member

dinhxuanvu commented Jul 29, 2020 •
edited
Loading

/lgtm so far. There are still a new comments from Evan & Alex need to be address and then this PR should be good to go.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
@gallettilance
Adding deprecated property type to bundles
dcc7e50 
Adding support marking bundles as deprecated and truncating the update
graph of a given package. Deprecated versions will not be installable.
Deprecating a bundle can result in the removal of channels but this is
not permitted for the default channel. Bundles that are not in the
index will be ignored during deprecation.
@kevinrizza
Copy link
Member

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gallettilance, kevinrizza, shawn-hurley

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020
@gallettilance
Copy link
Member Author

/retest

@ecordell
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
@gallettilance
Copy link
Member Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3da2d18 into operator-framework:master Jul 30, 2020
@cdjohnson
Copy link
Contributor

@gallettilance I realize this was merged quite some time ago, but why wasn't the deprecatetruncate command added to the opm registry commands?

@cdjohnson cdjohnson mentioned this pull request Jan 21, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecordell ecordell

@benluddy benluddy

@njhale njhale

@awgreene awgreene

@dinhxuanvu dinhxuanvu

@kevinrizza kevinrizza

@shawn-hurley shawn-hurley

@exdx exdx

Assignees

@ecordell ecordell

@kevinrizza kevinrizza

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet