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

git-clone: Add configurable short-commit result #1445

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

samdoran
Copy link
Contributor

It's helpful to have a short commit SHA for use in other tasks, such as apply-tags. The length of short-commit is also configurable, giving greater flexibility to the pipeline author.

@samdoran samdoran force-pushed the git-clone-add-short-commit branch from 2d4f493 to 29fe166 Compare September 18, 2024 21:37
@chmeliik
Copy link
Contributor

chmeliik commented Sep 20, 2024

@samdoran @jbpratt (#1457) what are your use cases?

IMO returning this type of metadata should not be the clone task's responsibility (nor should it have to return commit-timestamp #1040 (review), but we got bullied into accepting that one because "it is needed to unblock RHEL AI")

For custom git metadata needs, it would be best if you just ran whatever script you want in your own pipeline. But writing Tekton tasks can be annoying, so I understand why you would want this as a task result...

@chmeliik
Copy link
Contributor

Though to be fair, the technical limitation of returning all the git metadata one could possibly want should be fixable by enabling https://tekton.dev/docs/pipelines/tasks/#larger-results-using-sidecar-logs

And I suppose it would be nice for the clone task to provide all the results out of the box. It might not "do one thing well," but it would be convenient 😄

@chmeliik
Copy link
Contributor

/ok-to-test

@jbpratt
Copy link
Contributor

jbpratt commented Sep 20, 2024

Hey @chmeliik , here is the thread where I discussed my use case with others! https://redhat-internal.slack.com/archives/C031USXS2FJ/p1726817746562859?thread_ts=1726244072.482159&cid=C031USXS2FJ

@samdoran
Copy link
Contributor Author

what are your use cases?

My use case is similar to what @jbpratt described with a few extra caveats. The short commit hash is available in the release pipeline, so we use that to tag release image. It would be nice to have parity between test and release image tags.

I want to reference the Konflux build images (redhat-user-workloads) to run integration tests. For reasons, we use the image tag as a name for a clowder job invocation. That name is limited to 63 characters (also for reasons?), so bonfire deploy fails because the name field is too long.

I would prefer if this was avalible in PAC as a dynamic variable. But I don't have the ability to change that (since it requires a code change and release) whereas I can modify Tekton tasks.

This is kinda gross but works (parameter to add-tags task): pr-{{ body.pull_request.number }}-$(tasks.clone-repository.results.short-commit)

For custom git metadata needs, it would be best if you just ran whatever script you want in your own pipeline. But writing Tekton tasks can be annoying, so I understand why you would want this as a task result

For another one of our applications, I wrote a custom task to run a script to get the desired version and that was quite painful.

I don't mind making my own task and keeping it to myself, but it seems like having the short SHA would be beneficial to others. Especially since many of us are coming from app-interface where we have a long history of using the short commit SHA in many places.

@samdoran
Copy link
Contributor Author

I'll address the failing tests.

@samdoran samdoran force-pushed the git-clone-add-short-commit branch 3 times, most recently from c676f13 to 8cb9637 Compare September 20, 2024 16:12
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Ack, your use cases make sense. The prevailing sentiment seems to be that adding custom code to a Tekton pipeline is a PITA, which I can't really refute 😅

LGTM 👍

@chmeliik
Copy link
Contributor

Just need to re-generate git-clone-oci-ta again

@samdoran
Copy link
Contributor Author

I fixed the failures related to git-clone-oci-ta.

@chmeliik
Copy link
Contributor

Ah, one more thing. @samdoran if you don't mind, could you squash the commits?

@samdoran
Copy link
Contributor Author

Sure thing!

Also add a parameter to allow customizing the short commit length
Add short-commit to git-clone-oci-ta task as well
Update READMEs
@samdoran samdoran force-pushed the git-clone-add-short-commit branch from 4398107 to d771adf Compare September 20, 2024 18:28
@gbenhaim gbenhaim added this pull request to the merge queue Sep 22, 2024
Merged via the queue into konflux-ci:main with commit cccb488 Sep 22, 2024
13 checks passed
@samdoran
Copy link
Contributor Author

Thank you for merging and all the great reviews!

@samdoran samdoran deleted the git-clone-add-short-commit branch September 23, 2024 13:50
@samdoran samdoran restored the git-clone-add-short-commit branch September 23, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants