-
Notifications
You must be signed in to change notification settings - Fork 38
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
RHOAIENG-12524: fix(script): Improve generate-metadata-yaml.sh based on feedback #493
RHOAIENG-12524: fix(script): Improve generate-metadata-yaml.sh based on feedback #493
Conversation
Please note due to the unresolved discussions on the exact requirements of the It seems like
|
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.
Thank you for the followup changes, Andy! No objections from my side.
/lgtm
We still don't know if the script does what is required, but there's no reason to keep the PR hanging. It may be have to be revisited when we get more instructions from up above /lgtm |
/retest-required |
This commit is a follow up from discussions on opendatahub-io#472. The following improvements have been made to the script: - Set the executable bit on the script - Default values provided for all variables now (including `output_file`) so the script can and should be ran simply as `./generate-component-metadata.sh` - Default name now honors the guidance given via a [Slack discussion](https://redhat-internal.slack.com/archives/C05NXTEHLGY/p1734032874172439?thread_ts=1731327116.001979&cid=C05NXTEHLGY) - i.e. `Kubeflow Notebook Controller` - Any and all issues reported by `shellcheck` have been resolved - Comments have been added for the file as a whole, as well as on each function, to help improve understanding of the script and its implementation. The "comment template" used is a continuation from the one I experiemented with in opendatahub-io#484 Related-to: https://issues.redhat.com/browse/RHOAIENG-12524
3bea75e
to
5219e3f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
=======================================
Coverage ? 33.15%
=======================================
Files ? 2
Lines ? 941
Branches ? 0
=======================================
Hits ? 312
Misses ? 598
Partials ? 31 ☔ View full report in Codecov by Sentry. |
/lgtm |
/lgtm |
1 similar comment
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyatmiami 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 |
https://issues.redhat.com/browse/RHOAIENG-16752
Description
This commit is a follow up from discussions on #472.
The following improvements have been made to the script:
output_file
) so the script can and should be ran simply as./generate-component-metadata.sh
Kubeflow Notebook Controller
shellcheck
have been resolvedRelated-to: https://issues.redhat.com/browse/RHOAIENG-12524
How Has This Been Tested?
./generate-metadata-yaml.sh
was executed multiple times leveraging a variety of flags to ensure proper behavior.Merge criteria: