-
Notifications
You must be signed in to change notification settings - Fork 128
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
A new task to generate labels #1443
Conversation
e1f420b
to
3bda04f
Compare
3bda04f
to
7e4c7ba
Compare
90a25b5
to
62e7614
Compare
897f212
to
7d5587b
Compare
There has been more interest in this type of functionality from users (i.e. to set/forward the git tags so that they can be used at release time). Should we add this to the default pipelines? @mmorhun , have there been any criteria defined for when to add tasks to the default pipelines? |
I would love the ability to pass in a list of I'm not as clear on the value of the What would provide more utility and flexibility is a task parameter for an executable in the repository that returns a JSON list of |
The specific values mentioned are just those that it helps to interpolate. I think that it should be possible to modify this task parameters to add any variables that you want. For example, would something like this work? - name: label-templates
value:
- "release=$SOURCE_DATE_EPOCH"
- "build-date=$SOURCE_DATE"
- "short-commit=$(tasks.git-clone.results.short-commit)"
- name: source-date-epoch
value: '$(tasks.git-clone.results.commit-timestamp) (well, This task can be used to inject content into labels (from other tasks' results, for example). This does provide some indirection ... but if you don't need any of the label templates then you can also use the new parameter in the buildah task to directly add desired labels without having to use an ARG-based indirection. |
It seems like the |
7d5587b
to
100d234
Compare
This is tricky, because executing arbitrary code before the build voids the "trust" of the build pipeline. It would be doable in a trusted-artifacts pipeline - it would have to be a task that takes the source-code artifact as input but doesn't return a source-code artifact as output (any potential modifications would be local-only). And rather than running an executable from the source repo, the task could just execute any arbitrary script passed in (the script would run with the source repo as PWD, so could execute anything in the repo if needed). Maybe we should just add copy-and-pasteable snippets to the docs showing how to insert custom tasks into a trusted-artifacts pipeline in the least painful way |
Yeah, perhaps if we ran that arbitrary executable with |
I figured there would be some challenges with running arbitrary code. I'm doing this today in a custom task with a trusted artifact and it meets our needs while not compromising the integrity of the build artifact. Perhaps this is something best left to individual pipeline authors to implement as needed.
That's probably the best solution. It took me way more time than I would like to admit to get something so "simple" working, especially with the trusted artifacts model. |
100d234
to
cc320af
Compare
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.
looks reasonable to me. Thanks.
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.
A couple minor improvements, otherwise LGTM
8e093b5
to
65e78a1
Compare
b978eda
to
6a5cae9
Compare
build_args+=("$@") | ||
|
||
LABELS=() | ||
# Split `args` into two sets of arguments. |
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.
It looks complicated.
Could we have additional labels in a dedicated env var for example?
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.
I started that way @mmorhun, but if we make the labels available to the script as an env var, then I don't think I can make the param
an array
.
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.
Will a delimiter in the env var work? Like label1=value1|label2=something else
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.
Label values are too free-form for delimiters to work
Ralph's original approach was to use a JSON string of an array, but that's much less user-friendly
You can't really make something like this work with a JSON string
params:
- name: LABELS
value:
- $(tasks.generate-labels.results.LABELS[*])
- foo=bar
shift | ||
while [[ $# -gt 0 && $1 != --* ]]; do | ||
LABELS+=("--label" "$1") | ||
shift |
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.
Indentation issue.
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.
I think that's just the TA task generation doing some formatting ¯\_(ツ)_/¯
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.
LGTM, please squash into a number of commits that you consider appropriate before merging
And introduce a new generate-labels task. For https://issues.redhat.com/browse/KONFLUX-4274
c82a1e7
to
6bfb98d
Compare
For https://issues.redhat.com/browse/KONFLUX-4274. Turns out people really do use those labels (see linked issues).
This can have a future connection with #1268 once we do that. I.e., use the commit timestamp for the release label and build-date, rather than the real world date. But, that's for another day...