Create a .yaml file with kubeflow version #472

dibryant · 2024-11-26T16:05:50Z

Fixes for https://issues.redhat.com/browse/RHOAIENG-12524

Description

Collaborated with @andyatmiami
Created a .yaml file with kubeflow version with automated doc

How Has This Been Tested?

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

components/notebook-controller/config/.component_metadata.yamyaml

jiridanek · 2024-11-26T16:45:47Z

the yaml file looks good to me, and the script is (in my view) a little superfluous here (since it does not help much in creating the yaml, it can't even output the version, which has to be specified manually in an override) but it is not harmful in any way and it can be improved further

/lgtm
ship it!

andyatmiami · 2024-11-26T17:24:40Z

It does not need to be specified manually @jiridanek ...

The discrepancy here is such:

Our JIRA issue "example" file has version listed as 1.9.0
the VERSION file @atheo89 pointed us to - when parsed (and automatically handled by the script w/out override) - has v1.9.0

The feedback I was looking for was.... do we want to post-process the contents of VERSION to drop the leading v ? or do we really not care - and a version: field of v1.9.0 is "good enough"

the script will run without any argument other than -o and generate a valid component_metadata.yaml file.. its just not character-for-character identical to the "reference sample" we have in the JIRA ticket...

I can certainly add more code to make it match character for character - I just wasn't sure how LITERAL we wanted to treat the "reference sample"

but, here is what happens when running script with only the -o argument

./generate-metadata-yaml.sh -o /Users/astonebe/Development/Code/GitHub/odhio-kubeflow/components/notebook-controller/config/component_metadata.yaml

releases: 
  - name: kubeflow.org notebook-controller
    version: v1.9.0 
    repoUrl: https://github.com/kubeflow/kubeflow

if it is the desire of folks to match EXACTLY what was specified in the JIRA ticket for these values - I can absolutely do so... I had opted on the initial logic I provided to err on the side of simplicity (i.e. minimal post-processing of elements already present in the PROJECT file) 🤷

I have followed up in Slack here hoping to get some clarification:

https://redhat-internal.slack.com/archives/C05NXTEHLGY/p1732642635120689?thread_ts=1731327116.001979&cid=C05NXTEHLGY

But certainly the intention of the script is NOT to require passing these command-line overrides for the component_metadata.yaml fields.. I just included them for utmost flexibility given uncertainty on actual standards around these field names...

jstourac · 2024-11-27T07:33:13Z

components/notebook-controller/generate-metadata-yaml.sh

+
+        if ! [ -e "${output_file}" ]; then
+          touch "${output_file}"
+        fi


indentation?

jstourac · 2024-11-27T07:35:12Z

components/notebook-controller/generate-metadata-yaml.sh

+
+_parse_opts "$@"
+
+if [ -z "${output_file}" ]; then


just wondering why we don't have a default here?

you could argue that the script should not take any parameters and everything should be defaulted (that's my view), but the aim of the jira is to create the metadata yaml and this script is just a little extra bonus, so it's important to review the yaml and the rest is just cherry on top; it's not even run automatically, so it's as if it was not even there

Very fair comment...

I had (perhaps wrongly) designed with the intention of being able to be executed by any team (not just IDE team)...

Under that scenario - the "where" to place the file then becomes a bit ambiguous..

that being said - I undermine my own argument here since I rely on the relative positioning of the file in the repository to check for a PROJECT file - so a similar mechanism can be done for the default target location

jstourac · 2024-11-27T07:40:32Z

Pardon my ignorance - what is the use-case to use the attached bash script? And when to do so and how? I mean - shall it be done manually, shall it be part of the Makefile target? Shall we put some info somewhere?

Also, I'm not sure how much value it has since the resulting file is just three lines and there is no explanation how the script helps. But I don't say it's a problem. We just may need to harness it into our release process, e.g. via a Makefile target or something like that.

I put some comments but otherwise the code is nice and clean. Thank you.

From the jira issue:

unit testing: a test for checking the values match from Version file and details file

Are there any plans on this?

atheo89 · 2024-11-27T11:40:02Z

Hello there, I tested locally using the option -v like:
./generate-metadata-yaml.sh -o /home/atheodor/Downloads/kubeflow-rhoaieng-12524/components/notebook-controller/config/component_metadata.yaml -v 1.77.3 and it updated the component_metadata.yaml successfully.

However, if I change the version on the releasing/version/VERSION file and use it like: ./generate-metadata-yaml.sh -o /home/atheodor/Downloads/kubeflow-rhoaieng-12524/components/notebook-controller/config/component_metadata.yaml I get the following (I guess because i downloaded the PR)

fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
cat: /releasing/version/VERSION: No such file or directory
version attribute not specified and unable to be inferred

jiridanek · 2024-11-27T11:52:09Z

Can you try running git init first, maybe, in the directory where the git repo would be? I'll try this too.

Edit: just in case someone does not use this, a quick way to get the PR locally is with the gh tool, top right on this page, there is

andyatmiami · 2024-11-27T12:40:48Z

@jstourac :

We just may need to harness it into our release process, e.g. via a Makefile target or something like that.

yes, the intention would be to tie this into the release automation work at some point in the future..

and there is no explanation how the script helps

can you help me understand where such as explanation should be documented that would alleviate this concern?

jstourac · 2024-11-27T13:21:08Z

can you help me understand where such as explanation should be documented that would alleviate this concern?

In this context - some brief comment at the beginning of the script would do. Anyway, as Jiri pointed out - we probably want to get this in for the yaml file being there and the script itself is just a bonus and we can tweak it later.

jiridanek · 2024-11-27T13:26:08Z

+1, nobody said that one jira = one PR, there can be more

andyatmiami · 2024-11-27T13:34:49Z

sounds good - i will take the (great) feedback relevant to the script and spin out a separate PR on incorporating those suggestions. 🙇

openshift-ci · 2024-11-28T13:46:44Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

atheo89 · 2024-11-28T13:46:49Z

/lgtm

atheo89 · 2024-11-28T13:47:07Z

/override ci/prow/odh-notebook-controller-e2e

openshift-ci · 2024-11-28T13:47:12Z

@atheo89: Overrode contexts on behalf of atheo89: ci/prow/odh-notebook-controller-e2e

In response to this:

/override ci/prow/odh-notebook-controller-e2e

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-sigs/prow repository.

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` - 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

jiridanek · 2024-12-16T13:38:59Z

components/notebook-controller/config/component_metadata.yaml

@@ -0,0 +1,4 @@
+releases: 
+  - name: Kubeflow Notebook


This should be Kubeflow Notebook Controller, according to the Slack thread.

openshift-ci bot requested review from jiridanek and jstourac November 26, 2024 16:06

jiridanek reviewed Nov 26, 2024

View reviewed changes

components/notebook-controller/config/.component_metadata.yamyaml Outdated Show resolved Hide resolved

Create a .yaml file with kubeflow version

989d636

dibryant force-pushed the rhoaieng-12524 branch from 7b777a5 to 989d636 Compare November 26, 2024 16:39

openshift-ci bot assigned jiridanek Nov 26, 2024

openshift-ci bot added the lgtm label Nov 26, 2024

jstourac reviewed Nov 27, 2024

View reviewed changes

components/notebook-controller/generate-metadata-yaml.sh

if ! [ -e "${output_file}" ]; then

touch "${output_file}"

fi

Copy link

Member

jstourac Nov 27, 2024

Choose a reason for hiding this comment

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

indentation?

jstourac reviewed Nov 27, 2024

View reviewed changes

atheo89 added the approved label Nov 28, 2024

openshift-ci bot assigned atheo89 Nov 28, 2024

openshift-merge-bot bot merged commit e24c9e5 into opendatahub-io:main Nov 28, 2024
14 checks passed

andyatmiami mentioned this pull request Dec 9, 2024

RHOAIENG-12524: fix(script): Improve generate-metadata-yaml.sh based on feedback #493

Open

3 tasks

jiridanek reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a .yaml file with kubeflow version #472

Create a .yaml file with kubeflow version #472

dibryant commented Nov 26, 2024

jiridanek commented Nov 26, 2024 •

edited

Loading

andyatmiami commented Nov 26, 2024 •

edited

Loading

jstourac Nov 27, 2024

jstourac Nov 27, 2024

jiridanek Nov 27, 2024

andyatmiami Nov 27, 2024

jstourac commented Nov 27, 2024 •

edited

Loading

atheo89 commented Nov 27, 2024

jiridanek commented Nov 27, 2024 •

edited

Loading

andyatmiami commented Nov 27, 2024

jstourac commented Nov 27, 2024

jiridanek commented Nov 27, 2024

andyatmiami commented Nov 27, 2024

openshift-ci bot commented Nov 28, 2024

atheo89 commented Nov 28, 2024

atheo89 commented Nov 28, 2024

openshift-ci bot commented Nov 28, 2024

jiridanek Dec 16, 2024

Create a .yaml file with kubeflow version #472

Create a .yaml file with kubeflow version #472

Conversation

dibryant commented Nov 26, 2024

Description

How Has This Been Tested?

Merge criteria:

jiridanek commented Nov 26, 2024 • edited Loading

andyatmiami commented Nov 26, 2024 • edited Loading

jstourac Nov 27, 2024

Choose a reason for hiding this comment

jstourac Nov 27, 2024

Choose a reason for hiding this comment

jiridanek Nov 27, 2024

Choose a reason for hiding this comment

andyatmiami Nov 27, 2024

Choose a reason for hiding this comment

jstourac commented Nov 27, 2024 • edited Loading

atheo89 commented Nov 27, 2024

jiridanek commented Nov 27, 2024 • edited Loading

andyatmiami commented Nov 27, 2024

jstourac commented Nov 27, 2024

jiridanek commented Nov 27, 2024

andyatmiami commented Nov 27, 2024

openshift-ci bot commented Nov 28, 2024

atheo89 commented Nov 28, 2024

atheo89 commented Nov 28, 2024

openshift-ci bot commented Nov 28, 2024

jiridanek Dec 16, 2024

Choose a reason for hiding this comment

jiridanek commented Nov 26, 2024 •

edited

Loading

andyatmiami commented Nov 26, 2024 •

edited

Loading

jstourac commented Nov 27, 2024 •

edited

Loading

jiridanek commented Nov 27, 2024 •

edited

Loading