-
Notifications
You must be signed in to change notification settings - Fork 65
🐛 desired state in applier #1539
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
🐛 desired state in applier #1539
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
d6a06d3
to
c5c48bc
Compare
b7b70ca
to
52fd265
Compare
313ce3d
to
8ef484b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1539 +/- ##
==========================================
+ Coverage 74.25% 75.64% +1.38%
==========================================
Files 42 42
Lines 3329 3326 -3
==========================================
+ Hits 2472 2516 +44
+ Misses 676 638 -38
+ Partials 181 172 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @azych
That seems great !!!
Well done 🥇
Really thank you for adding the tests.
Just a small nits and questions.
8ef484b
to
261bc08
Compare
261bc08
to
8060281
Compare
@camilamacedo86 @joelanford @bentito I think any potential discussion about #1539 (comment) should not be a blocker and can be had outside (maybe on an issue?) if necessary, WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🥇
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome! Thanks for the thorough unit testing ^^
A few minor fixes:internal/applier/helm.go:getReleaseState() contained two
if
blocks that handleddriver.ErrReleaseNotFound
with the first one returning, which meant the second block would never run.Additionally, the first handler always returned a
nil
desiredState
causing the only currently usedPreflight
validator (crdupgradesafety
) and potentially others in the future to exit immediately because of not having anything to validate.This fix leaves only the second handler block in, which actually triggers a dry-run installation and is able to return a proper
desiredState
that anyPreflight
validation can work with. It also has the added benefit of potentially spotting any issues and exiting earlier in case the dry-run failed.2. hack/tools/catalogs/download-catalog script - changes the name of the catalogd service fromcatalogd-server
to the currently usedcatalogd-service
.3. hack/tools/catalogs/list-compatible-bundles script - adds the missingextract-olm-package-property
function to the call chain and uses the correct name of regex filtering function (filter-by-regex-if-necessary
), making sure the script works.The main issues here were two:
1.
regex_it
did not exist and was renamed during review of the original PR tofilter-by-regex-if-necessary
2.
extract-olm-package-property
call was missing from the chain meaning the filter function couldn't recognize the structure it was expectingFixes to
hack/tools/catalogs/
scripts have been separated from this PR into PRs of their own: #1546 and #1545 as per #1539 (comment)Reviewer Checklist