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

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged
merged 2 commits into from
Sep 11, 2024
Merged

🌱 Descriptive Upgrade Constraint Policy constants #1233

merged 2 commits into from
Sep 11, 2024

Conversation

skattoju
Copy link
Contributor

@skattoju skattoju commented Sep 9, 2024 •
edited by joelanford
Loading

Fixes #1151

This PR renames the constant values used in the API for UpgradeConstraintPolicy and updates their references in the documentation.

@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 Sep 9, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 9, 2024 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 8f9b615
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/66e1951a3fb51b0008e49163
😎 Deploy Preview https://deploy-preview-1233--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@skattoju skattoju changed the title Descriptive Upgrade Constraint Policy constants 🌱 Descriptive Upgrade Constraint Policy constants Sep 9, 2024
@skattoju skattoju force-pushed the desc_upgradeconstraintpolicy branch from f4ecb7c to eb4ca3f Compare September 9, 2024 14:22
@skattoju skattoju marked this pull request as ready for review September 9, 2024 14:25
@skattoju skattoju requested a review from a team as a code owner September 9, 2024 14:25
@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 Sep 9, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.49%. Comparing base (57ac405) to head (8f9b615).
Report is 10 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1233      +/-   ##
==========================================
- Coverage   76.53%   76.49%   -0.05%     
==========================================
  Files          40       40              
  Lines        2340     2336       -4     
==========================================
- Hits         1791     1787       -4     
  Misses        392      392              
  Partials      157      157
Flag Coverage Δ
e2e 57.57% <100.00%> (-0.08%) ⬇️
unit 52.39% <100.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven
everettraven requested changes Sep 9, 2024
View reviewed changes
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.

Only one change that stood out to me as needing to be made

@skattoju skattoju force-pushed the desc_upgradeconstraintpolicy branch 2 times, most recently from 9adaa25 to bc22eda Compare September 9, 2024 16:30
LalatenduMohanty
LalatenduMohanty previously approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@LalatenduMohanty LalatenduMohanty 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 Sep 9, 2024
@LalatenduMohanty
Copy link
Member

Conflicts because PR #1237 has merged

everettraven
everettraven previously approved these changes Sep 9, 2024
View reviewed changes
@skattoju skattoju force-pushed the desc_upgradeconstraintpolicy branch from bc22eda to f6da41e Compare September 9, 2024 20:40
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2024
@everettraven
Copy link
Contributor

Approved and then looked at CI - looks like you need to:

  • rebase and resolve merge conflicts
  • regenerate manifests with make manifests

@skattoju
Copy link
Contributor Author

skattoju commented Sep 9, 2024

I've rebased and resolved conflicts. The CI failures are because we are changing the API right ?

@everettraven
Copy link
Contributor

@skattoju It looks like there is still a sanity failure. I do expect the go-apidiff failure though.

@everettraven
Copy link
Contributor

Looks like the sanity failure is due to the newly added types documentation

@skattoju skattoju force-pushed the desc_upgradeconstraintpolicy branch from f6da41e to 514c1a2 Compare September 10, 2024 13:58
@skattoju
Copy link
Contributor Author

looks like sanity is passing after doing make verify and committing changes to docs/refs/api/operator-controller-api-reference.md 👍

@skattoju @joelanford
change match labels to olm.operatorframework.io/metadata.name
8f9b615 
fix unintentional override due to error in rebase

Co-authored-by: Joe Lanford <[email protected]>
LalatenduMohanty
LalatenduMohanty approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@LalatenduMohanty LalatenduMohanty 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 Sep 11, 2024
@joelanford joelanford added this pull request to the merge queue Sep 11, 2024
Merged via the queue into operator-framework:main with commit 78a6c00 Sep 11, 2024
18 of 20 checks passed
@skattoju skattoju mentioned this pull request Sep 25, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LalatenduMohanty LalatenduMohanty LalatenduMohanty approved these changes

@joelanford joelanford joelanford approved these changes

@everettraven everettraven everettraven left review comments

Assignees

@LalatenduMohanty LalatenduMohanty

Labels
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.

Update the allowed values of the ClusterExtension.Spec.UpgradeConstraintPolicy to be more descriptive
4 participants
@skattoju @LalatenduMohanty @everettraven @joelanford