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

Sort channels in lexicographical order in packagemanifest #2925

Merged
merged 1 commit into from
Feb 28, 2023
Merged

Sort channels in lexicographical order in packagemanifest #2925

merged 1 commit into from
Feb 28, 2023

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Feb 22, 2023 •
edited

Motivation for the change:
See operator-framework/operator-registry#1069 for more details

Architectural changes:
None

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2023
@anik120 anik120 force-pushed the packageserver-sort-channel-entries branch from 9a410ec to 99b0c51 Compare February 23, 2023 16:56
@anik120 anik120 changed the title WIP: sort channels in lexicographical order in packagemanifest Sort channels in lexicographical order in packagemanifest Feb 23, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2023
@joelanford
Copy link
Member

Nice! Should we add a test for this to avoid regressions?

@anik120 anik120 force-pushed the packageserver-sort-channel-entries branch 2 times, most recently from 4187bc5 to 33b6aa9 Compare February 27, 2023 16:32
@anik120
Copy link
Contributor Author

anik120 commented Feb 27, 2023

Nice! Should we add a test for this to avoid regressions?

@joelanford done 👍🏽 PTAL.

joelanford
joelanford reviewed Feb 27, 2023
View reviewed changes
pkg/package-server/provider/registry_test.go Outdated
Comment on lines 484 to 487
{
Name: "alpha",
CsvName: "etcdoperator.v0.9.2",
},
{
Name: "beta",
CsvName: "etcdoperator.v0.10.1",
},
Copy link
Member

Choose a reason for hiding this comment

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

Does this test pass without the sort change? Seems like we may want to reverse the order of these here and check that they always end up as [alpha, beta].

Copy link
Member

Choose a reason for hiding this comment

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

Reason being: pkg.GetChannels() returns in whatever order we define here I think:

https://github.com/operator-framework/operator-registry/blob/0a7ff74f9fc6c92be12a48a0f8e67ecfb6a8e60e/pkg/api/registry.pb.go#L175-L180

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I've reversed the order, the input is now [beta,alpha] with the expected output as [alpha, beta]. And with this setup, when the sort change is reversed, this test fails.

@anik120 anik120 force-pushed the packageserver-sort-channel-entries branch from 33b6aa9 to f06ebaf Compare February 27, 2023 19:09
joelanford
joelanford approved these changes Feb 27, 2023
View reviewed changes
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

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2023
@perdasilva perdasilva force-pushed the packageserver-sort-channel-entries branch from f06ebaf to ca055ba Compare February 28, 2023 09:37
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 28, 2023

New changes are detected. LGTM label has been removed.

@perdasilva perdasilva added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: anik120, joelanford

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

@perdasilva perdasilva force-pushed the packageserver-sort-channel-entries branch from ca055ba to 65e90aa Compare February 28, 2023 13:00
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 28, 2023

New changes are detected. LGTM label has been removed.

@perdasilva perdasilva added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023
@anik120 @perdasilva
Sort channels in lexicographical order in Packagemanifest
3b1b3f9 
See operator-framework/operator-registry#1069
for more details

Signed-off-by: Anik <anikbhattacharya93@gmail.com>
@perdasilva perdasilva force-pushed the packageserver-sort-channel-entries branch from 65e90aa to 3b1b3f9 Compare February 28, 2023 14:02
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 28, 2023

New changes are detected. LGTM label has been removed.

@perdasilva perdasilva added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023
@perdasilva perdasilva merged commit e4b1532 into operator-framework:master Feb 28, 2023
@grokspawn grokspawn mentioned this pull request Mar 31, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford approved these changes

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

Assignees

@joelanford joelanford

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
3 participants
@anik120 @joelanford @perdasilva