Skip to content

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Jan 6, 2025

Description

I think when I addressed some reviewer comments I didn't do a good job and mangled the script. This fixes it. I've tested it manually and now it works as advertised.

Adding from #1539 to keep track the history of this change done by @azych

hack/tools/catalogs/list-compatible-bundles script - adds the missing extract-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 to filter-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 expecting

Before change result:

➜ operator-controller git:(main) ✗ ./hack/tools/catalogs/list-compatible-bundles < operatorhubio-catalog.json

./hack/tools/catalogs/list-compatible-bundles: line 80: regex_it: command not found
NOTE: OLM v1 currently only supports bundles that support AllNamespaces install model, don't have dependencies, and don't contain webhooks
WARNING: These results may include bundles with webhooks, which are incompatible`

After change result:

➜ operator-controller git:(main) ✗ ./hack/tools/catalogs/list-compatible-bundles < operatorhubio-catalog.json > filtered.json
NOTE: OLM v1 currently only supports bundles that support AllNamespaces install model, don't have dependencies, and don't contain webhooks
WARNING: These results may include bundles with webhooks, which are incompatible

➜ operator-controller git:(main) cat filtered.json
[
{
"packageName": "ack-acm-controller",
"versions": [
"0.0.1",
"0.0.10",
"0.0.12",
"0.0.15",
"0.0.16",
"0.0.17",
"0.0.18",
"0.0.19",
"0.0.2",
"0.0.20",
"0.0.4",
"0.0.5",
"0.0.6",
"0.0.7",
"0.0.9",
"1.0.0"
]
},
...

@perdasilva perdasilva requested a review from a team as a code owner January 6, 2025 11:14
Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 82f238d
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/677bd1cbae494700084b8e4e
😎 Deploy Preview https://deploy-preview-1543--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.

@perdasilva perdasilva force-pushed the perdasilva/fix-script branch from bfe932b to 99f1eeb Compare January 6, 2025 11:15
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.22%. Comparing base (de6a813) to head (82f238d).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1543      +/-   ##
==========================================
- Coverage   74.25%   74.22%   -0.04%     
==========================================
  Files          42       42              
  Lines        3329     3329              
==========================================
- Hits         2472     2471       -1     
- Misses        676      677       +1     
  Partials      181      181              
Flag Coverage Δ
e2e 52.11% <ø> (ø)
unit 56.95% <ø> (-0.04%) ⬇️

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.

camilamacedo86
camilamacedo86 previously approved these changes Jan 6, 2025
Copy link
Contributor

@camilamacedo86 camilamacedo86 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 Jan 6, 2025
cat - | select-bundle-documents | that-support-allnamespace-install-mode | that-dont-have-dependencies | extract-olm-package-property | group-versions-by-package-name | filter-by-regex-if-necessary

echo "NOTE: OLM v1 currently only supports bundles that support AllNamespaces install model, don't have dependencies, and don't contain webhooks" >&2
echo "WARNING: These results may include bundles with webhooks, which are incompatible" >&2
Copy link
Contributor

Choose a reason for hiding this comment

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

@perdasilva NIT ^ We need to give a space at the end of the file here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/fix-script branch from 99f1eeb to 5fe5357 Compare January 6, 2025 12:05
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2025
Copy link

@gavinmbell gavinmbell left a comment

Choose a reason for hiding this comment

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

Nit: Can we remove the use of "function" as a keyword in defining bash functions, please

Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva
Copy link
Contributor Author

Nit: Can we remove the use of "function" as a keyword in defining bash functions, please

done

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

I believe it’s primarily used by us, right?
So since it does not affect the end users, it probably should be 🌱

However, since we're not using the emojis to organize the release notes, I’m not particularly concerned about it.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2025
@azych
Copy link
Contributor

azych commented Jan 6, 2025

@perdasilva you just took the fix I proposed in a still open PR (#1539) along with the description and created your own PR with those same exact changes and description, only difference being it is now authored by yourself ;)

I don't understand this and the reasoning behind doing that. It doesn't seem to be a critical fix which needs to be merged ASAP, as the script has not been functional for ~4 months.

@perdasilva
Copy link
Contributor Author

@perdasilva you just took the fix I proposed in a still open PR (#1539) along with the description and created your own PR with those same exact changes and description, only difference being it is now authored by yourself ;)

I don't understand this and the reasoning behind doing that. It doesn't seem to be a critical fix which needs to be merged ASAP, as the script has not been functional for ~4 months.

I didn't take the fix you proposed, or add a link to your PR in my original description. I fixed a problem I ran into before the break in parallel with you, without knowledge of your PR. Closing as duplicate.

@perdasilva perdasilva closed this Jan 7, 2025
@gavinmbell
Copy link

With this closed there is no addressing the s/function//g issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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