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

Add channel entries to the packagemanifests API #2893

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Add channel entries to the packagemanifests API #2893

merged 1 commit into from
Nov 21, 2022

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Nov 11, 2022 •
edited
Loading

Description of the change:
Add list of channel entries to the package manifest API, where each entry includes the CSV name and version.

Motivation for the change:
Surface list of available bundles, which can help users understand what can be installed (e.g. answer to queston: "what can i set startingCSV to?").

See also:

Architectural changes:
None. Only notable change is that the package server calls ListBundles to get this information when syncing a catalog source.

Testing remarks:

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 Nov 11, 2022
@openshift-ci
Copy link

openshift-ci bot commented Nov 11, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@joelanford joelanford force-pushed the packagemanifest-channel-entries branch 3 times, most recently from e9c7323 to f0ca220 Compare November 11, 2022 21:02
grokspawn
grokspawn reviewed Nov 11, 2022
View reviewed changes
pkg/package-server/provider/registry_test.go Outdated
@@ -512,6 +512,22 @@ func TestRegistryProviderGet(t *testing.T) {
require.NoError(t, json.Unmarshal([]byte(etcdCSVJSON), &csv))
return operators.CreateCSVDescription(&csv, etcdCSVJSON)
}(),
Entries: []operators.ChannelEntry{
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the entry list for test prometheus and etcd operators is reused throughout, does it make sense to declare them as objects which we can reuse?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@joelanford joelanford force-pushed the packagemanifest-channel-entries branch from f0ca220 to 143acfa Compare November 11, 2022 21:36
@joelanford joelanford marked this pull request as ready for review November 11, 2022 21:39
@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 Nov 11, 2022
perdasilva
perdasilva reviewed Nov 14, 2022
View reviewed changes
pkg/package-server/apis/operators/packagemanifest_types.go
@@ -81,6 +80,17 @@ type PackageChannel struct {

// CurrentCSVSpec holds the spec of the current CSV
CurrentCSVDesc CSVDescription

// Entries lists all CSVs in the channel, with their upgrade edges.
Entries []ChannelEntry
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would refactoring to Entries to ChannelEntries improve readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could go either way. We're already in the PackageChannel struct, and none of the other fields have a Channel prefix, so my instinct was just Entries.

I did originally have the struct type ChannelEntry called Entry. I changed from Entry to ChannelEntry in that case because the type is defined at the package level, and it made sense to me for usage of the struct from outside the package to look like operators.ChannelEntry rather than operators.Entry

@perdasilva
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2022
@joelanford joelanford force-pushed the packagemanifest-channel-entries branch from 143acfa to 3e8c8c3 Compare November 14, 2022 16:20
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2022
@openshift-ci
Copy link

openshift-ci bot commented Nov 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2022
@joelanford
add channel entries to the packagemanifests api
0a89489 
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the packagemanifest-channel-entries branch from 3e8c8c3 to 0a89489 Compare November 21, 2022 13:25
@awgreene
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit e7c2de3 into operator-framework:master Nov 21, 2022
@joelanford joelanford deleted the packagemanifest-channel-entries branch November 21, 2022 16:17
@joelanford joelanford mentioned this pull request Dec 6, 2022
Merged
11 tasks
@tlwu2013 tlwu2013 mentioned this pull request Mar 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perdasilva perdasilva perdasilva left review comments

@grokspawn grokspawn grokspawn approved these changes

@awgreene awgreene Awaiting requested review from awgreene

@gallettilance gallettilance Awaiting requested review from gallettilance

Assignees

@perdasilva perdasilva

@awgreene awgreene

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.
5 participants
@joelanford @perdasilva @awgreene @grokspawn @openshift-merge-robot