Skip to content

Conversation

@theishshah
Copy link
Contributor

@theishshah theishshah commented Feb 20, 2023

This PR contains the initial logic and docs for cleaning up the FBC Z-stream replacements. Follow up work for this PR will include a unit testing deep dive spike and some re-factoring to pre-sort and filter versions.

Closes #1031

streamlining & add'l namechanges to working utest, debug printfs removed
working idempotent approach across set of all channel entries; but gaps in utest and UGLY implementation refactor minimally-nested ifs
updated supporting docs
standardize on yaml.UnmarshallStrict to give better diagnostics for unexpected fields

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 20, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: theishshah
Once this PR has been reviewed and has the lgtm label, please assign njhale for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@grokspawn
Copy link
Contributor

Would you please retitle to something like OPECO-2596: semver should generate replaces across Y-streams in minor-channel mode and add solves #1031 ?

@theishshah theishshah changed the title Remove replaces-only approach in FBC OPECO-2596: semver should generate replaces across Y-streams in minor-channel mode, Closes #1031 Feb 20, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 20, 2023
@openshift-ci-robot
Copy link

@theishshah: This pull request references OPECO-2596 which is a valid jira issue.

In response to this:

This PR contains the initial logic and docs for cleaning up the FBC Z-stream replacements. Follow up work for this PR will include a unit testing deep dive spike and some re-factoring to pre-sort and filter versions.

streamlining & add'l namechanges to working utest, debug printfs removed
working idempotent approach across set of all channel entries; but gaps in utest and UGLY implementation refactor minimally-nested ifs
updated supporting docs
standardize on yaml.UnmarshallStrict to give better diagnostics for unexpected fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, great work @theishshah!

The reason for me marking this review as requesting changes is some minor things I found in how the tests are written that I think could be contributing to the test failures.

Aside from that I had a few smaller nits.

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@grokspawn grokspawn force-pushed the y-streams-for-all branch from 4093256 to e97cb5d Compare March 3, 2023 14:56
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023
@grokspawn grokspawn enabled auto-merge (squash) March 3, 2023 15:35
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #1065 (d7edf24) into master (fe51230) will increase coverage by 0.04%.
The diff coverage is 85.71%.

❗ Current head d7edf24 differs from pull request most recent head e970b8b. Consider uploading reports for the commit e970b8b to get more accurate results

@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
+ Coverage   52.75%   52.79%   +0.04%     
==========================================
  Files         106      107       +1     
  Lines        9378     9381       +3     
==========================================
+ Hits         4947     4953       +6     
+ Misses       3517     3511       -6     
- Partials      914      917       +3     
Impacted Files Coverage Δ
alpha/declcfg/write.go 75.38% <ø> (ø)
alpha/template/semver/types.go 80.00% <80.00%> (ø)
alpha/template/semver/semver.go 66.16% <86.11%> (+1.63%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@grokspawn grokspawn force-pushed the y-streams-for-all branch 2 times, most recently from b294591 to e5eb6fc Compare March 3, 2023 15:45
@grokspawn grokspawn requested a review from everettraven March 3, 2023 15:46
@grokspawn grokspawn changed the title OPECO-2596: semver should generate replaces across Y-streams in minor-channel mode, Closes #1031 OPECO-2596: semver should generate replaces across Y-streams in minor-channel mode Mar 3, 2023
@openshift-ci-robot
Copy link

@theishshah: This pull request references OPECO-2596 which is a valid jira issue.

In response to this:

This PR contains the initial logic and docs for cleaning up the FBC Z-stream replacements. Follow up work for this PR will include a unit testing deep dive spike and some re-factoring to pre-sort and filter versions.

Closes #1031

streamlining & add'l namechanges to working utest, debug printfs removed
working idempotent approach across set of all channel entries; but gaps in utest and UGLY implementation refactor minimally-nested ifs
updated supporting docs
standardize on yaml.UnmarshallStrict to give better diagnostics for unexpected fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@grokspawn grokspawn force-pushed the y-streams-for-all branch from e5eb6fc to 549a371 Compare March 3, 2023 15:52
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@grokspawn grokspawn force-pushed the y-streams-for-all branch from 549a371 to 207a970 Compare March 3, 2023 15:53
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@grokspawn grokspawn merged commit 6b1b166 into operator-framework:master Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opm alpha render-veneer semver missing replaces between minor channels

6 participants