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.

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

feat(ISV-5125): Add unique tag info to purl #548

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

wcheang
Copy link
Contributor

@wcheang wcheang commented Sep 5, 2024

As part of the release-time SBOM work, purls need to include a unique tag. The unique tag has been determined to be a tag of the format -. If such a tag does not exist, it will not be added to the purl.

@wcheang wcheang requested a review from a team as a code owner September 5, 2024 18:48
Copy link

openshift-ci bot commented Sep 5, 2024

Hi @wcheang. Thanks for your PR.

I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

Hey there, can you bump the version string at the top of populate-release-notes-images.yaml and add an entry to the README for the new version?

Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

Your unit tests failed. Unfortunately they don't run after you push until one of us hits approve and run, but I just did. Other than that, I have no changes to ask for

@wcheang
Copy link
Contributor Author

wcheang commented Sep 6, 2024

Your unit tests failed. Unfortunately they don't run after you push until one of us hits approve and run, but I just did. Other than that, I have no changes to ask for

So it looks like the tags var wasn't parsed in a format that bash can traverse. Since the var is used to form the jsonString later, I don't want to mess with that, so I created a new var called tagsElems with the right format for traversal. Could you please approve again so the tests will run?

@johnbieren
Copy link
Collaborator

johnbieren commented Sep 9, 2024

Your unit tests failed. Unfortunately they don't run after you push until one of us hits approve and run, but I just did. Other than that, I have no changes to ask for

So it looks like the tags var wasn't parsed in a format that bash can traverse. Since the var is used to form the jsonString later, I don't want to mess with that, so I created a new var called tagsElems with the right format for traversal. Could you please approve again so the tests will run?

It failed again. I don't think you need tagElems. You can just do something like we do to index the component right above it.

NUM_TAGS=$(jq '.tags | length' <<< $tags)
 for ((j = 0; j < $NUM_TAGS; j++)) ; do
  tag=$(jq -c --argjson j "$j" '.tags[$j]' <<< $tags)
  if [[ $tag =~ $uniqueTagRegex ]] && [[ ${#tag} > ${#uniqueTag} ]] ; then
....

should work

@wcheang
Copy link
Contributor Author

wcheang commented Sep 11, 2024

It failed again. I don't think you need tagElems. You can just do something like we do to index the component right above it.

NUM_TAGS=$(jq '.tags | length' <<< $tags)
 for ((j = 0; j < $NUM_TAGS; j++)) ; do
  tag=$(jq -c --argjson j "$j" '.tags[$j]' <<< $tags)
  if [[ $tag =~ $uniqueTagRegex ]] && [[ ${#tag} > ${#uniqueTag} ]] ; then
....

should work

Thank you for the suggestion, but I tried it out and it wouldn't work with how tags is currently defined. tags is read in from the json file as a String with jq -c '.tags', instead of jq -c '.tags[]'. I could update how tags is defined, but that might change the existing behavior when tags is added to the data.json at the end of the task. That is why I wanted to create a new var. If it is okay to change the existing tags var, then I can update it along with your suggestion.

@wcheang wcheang force-pushed the ISV-5125 branch 3 times, most recently from c46e342 to 51394a2 Compare September 11, 2024 21:56
Copy link
Contributor

@mmalina mmalina left a comment

Choose a reason for hiding this comment

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

I added a few nitpicks, but also, you will need to sign your commit. See here for more details: https://github.com/konflux-ci/release-service-catalog/blob/development/CONTRIBUTING.md#signing-commits

@johnbieren
Copy link
Collaborator

@wcheang are you able to make the changes the checkton check is requesting? If you click details on it, it underlines exactly what part of your code to change and tells you what to do to it. We are trying to abide by shellcheck in our tekton tasks

@johnbieren
Copy link
Collaborator

As far as the code itself, once Martin's 2 things are addressed, it lgtm

@wcheang wcheang force-pushed the ISV-5125 branch 3 times, most recently from b821558 to ee3ff95 Compare September 13, 2024 03:25
johnbieren
johnbieren previously approved these changes Sep 13, 2024
Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

needs rebase

mmalina
mmalina previously approved these changes Sep 17, 2024
@openshift-ci openshift-ci bot added the lgtm label Sep 17, 2024
@johnbieren
Copy link
Collaborator

@wcheang
Copy link
Contributor Author

wcheang commented Sep 17, 2024

Oh, I just noticed the commit isn't signed https://github.com/konflux-ci/release-service/blob/main/CONTRIBUTING.md#signing-commits

Sorry, it wasn't signed because I rebased it on Github....Amended it with signature locally.

@johnbieren
Copy link
Collaborator

Oh, I just noticed the commit isn't signed https://github.com/konflux-ci/release-service/blob/main/CONTRIBUTING.md#signing-commits

Sorry, it wasn't signed because I rebased it on Github....Amended it with signature locally.

It needs another rebase too. Once rebased, do you want it merged? I can turn it on auto merge if so

@wcheang
Copy link
Contributor Author

wcheang commented Sep 17, 2024

Oh, I just noticed the commit isn't signed https://github.com/konflux-ci/release-service/blob/main/CONTRIBUTING.md#signing-commits

Sorry, it wasn't signed because I rebased it on Github....Amended it with signature locally.

It needs another rebase too. Once rebased, do you want it merged? I can turn it on auto merge if so

Yes please merge this.

@johnbieren johnbieren enabled auto-merge (squash) September 17, 2024 15:08
auto-merge was automatically disabled September 18, 2024 14:11

Head branch was pushed to by a user without write access

@wcheang
Copy link
Contributor Author

wcheang commented Sep 18, 2024

@johnbieren somehow it needed rebase again before it was able to merge 🤦‍♀️ could you please approve and set auto-merge again.

@johnbieren
Copy link
Collaborator

@johnbieren somehow it needed rebase again before it was able to merge 🤦‍♀️ could you please approve and set auto-merge again.

Sorry about that, we had some hot fixes to get in today so those kind of took priority on the merges. Can you rebase one more time? I am around so I should be able to see it all the way through to merge (and we have nothing else about to be merged AFAIK)

As part of the release-time SBOM work, purls need to include a unique
tag. The unique tag has been determined to be a tag of the format
<version>-<timestamp>. If such a tag does not exist, it will not be
added to the purl.

Signed-off-by: Wai Cheang <wcheang@redhat.com>
Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@johnbieren
Copy link
Collaborator

/test release-pipelines-e2e-suite

@johnbieren johnbieren enabled auto-merge (squash) September 18, 2024 20:58
@johnbieren johnbieren merged commit 3144132 into konflux-ci:development Sep 18, 2024
6 checks passed
theflockers added a commit to theflockers/release-service-catalog that referenced this pull request Sep 19, 2024
this commit reverts the PR konflux-ci#548
Signed-off-by: Leandro Mendes <lmendes@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants