Skip to content

Explain unique replaces chain requirement in channel sort errors. #2260

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

Conversation

benluddy
Copy link
Contributor

The "multiple channel head" error that can be returned from channel
sorting has been a source of confusion for users. The error message
now explains that a unique replacement chain is required in order to
define the relative order between channel entries, and it also
provides an abbreviated list of all replacement chains identified, in
the form "head...tail" or "singleton".

@openshift-ci openshift-ci bot requested review from ecordell and hasbro17 July 16, 2021 14:06
@benluddy benluddy force-pushed the name-multiple-replaces-chains branch from 9df7885 to 0de3fd4 Compare July 16, 2021 14:06
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2021
@benluddy benluddy mentioned this pull request Jul 16, 2021
5 tasks
@benluddy
Copy link
Contributor Author

/hold

This is a WIP, and may have also identified a bug with one of the new tests.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2021
@benluddy benluddy force-pushed the name-multiple-replaces-chains branch from 0de3fd4 to a2ca9b0 Compare July 16, 2021 14:28
@benluddy benluddy force-pushed the name-multiple-replaces-chains branch from a2ca9b0 to 4822746 Compare July 16, 2021 17:58
The "multiple channel head" error that can be returned from channel
sorting has been a source of confusion for users. The error message
now explains that a unique replacement chain is required in order to
define the relative order between channel entries, and it also
provides an abbreviated list of all replacement chains identified, in
the form "head...tail" or "singleton".

Signed-off-by: Ben Luddy <[email protected]>
@benluddy benluddy force-pushed the name-multiple-replaces-chains branch from 4822746 to 6391025 Compare July 16, 2021 18:10
@benluddy
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2021
Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

/lgtm

also discussed offline the merits of coupling the message change with the test fixes; but both can probably reasonably be backported the same degree

}

// TODO: do we care if the channel doesn't include every bundle in the input?
if len(chains) == 0 {
// Bug?
Copy link
Member

Choose a reason for hiding this comment

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

programmer error if we hit this

"packageB.v1": genOperator("packageB.v1", "1.0.0", "", "packageB", "alpha", "community", "olm", nil, nil, nil, "", false),
}
assert.Equal(t, 2, len(operators))
assert.Len(t, operators, 1)
Copy link
Member

Choose a reason for hiding this comment

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

discussed offline: this change confused me, but the old assertions were incorrect because packageA.v1 replaced itself

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, ecordell

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-merge-robot openshift-merge-robot merged commit 014bd31 into operator-framework:master Jul 16, 2021
@jianzhangbjz
Copy link
Contributor

@benluddy @kevinrizza Will it be cherry-picked to the downstream repo? If yes, when? I checked the latest commits(1dc76f0) of the downstream repo, but not found this PR. I am really interested in the process of picking up upstream PRs for the downstream. Could you help explain more about it? Or any doc? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants