-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Issue 9032 - Add support for step display name #9033
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
base: main
Are you sure you want to change the base?
Issue 9032 - Add support for step display name #9033
Conversation
6510629
to
fe8cd47
Compare
/kind feature |
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.
Thanks for this!
Just a small comment about tests, otherwise it looks good.
description: The sum of the two provided integers | ||
steps: | ||
- name: sum | ||
displayName: Do the sum |
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.
Thanks for adding this to the example.
I think it would be good to have this is some more tests for the various cases:
- displayName set in an inline step in a Task
- displayName set in an inline step embedded in a Pipeline or TaskRun, or else
Could you add this to the v1beta1 tests too please?
It would be nice to use displayName somewhere in unit tests too, at least the task conversion one?
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.
Yes, I will try to add more tests 👍
I was able to test in my cluster that the resolver returned the displayName of the step, but then I cannot find the displayName of the step in the taskrun. Can you indicate me in the code where/how the TaskRun is created with the resolver content please ?
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.
@afrittoli I added some tests but I would like help to understand the errors reported by the CI
docs/stepactions.md
Outdated
- [`workingDir`](#declaring-workingdir) | ||
- [`securityContext`](#declaring-securitycontext) | ||
- [`volumeMounts`](#declaring-volumemounts) | ||
- [`displayName`](#declaring-displayName) |
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.
Ah, nice, we already had this on the StepActions, but no documented, thanks for fixing that
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.
To be honest, I thought my changes will also apply to StepAction as they are semantically Step.
I can't find StepAction in pipeline/pkg/apis/pipeline/v1
to try to apply displayName
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.
StepActions are only in beta for now, and they already include a description field
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 was thinking about displayName
, not description
😃
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.
Right, the field name is different, but description
is meant as user-facing text to UIs, so it feels like having both would be redundant. Description is also missing in the docs.
pipeline/pkg/apis/pipeline/v1beta1/stepaction_types.go
Lines 104 to 107 in d97e81a
// Description is a user-facing description of the stepaction that may be | |
// used to populate a UI. | |
// +optional | |
Description string `json:"description,omitempty"` |
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 added description field in doc 👍
@valAndre07 Thank you for this PR. Please make sure that all of your changes in the end are in one commit and you are following the commit message standards and are using conventional commit messages. https://github.com/tektoncd/community/blob/main/standards.md#commit-messages |
ac7bde9
to
daa90a3
Compare
@afrittoli @waveywaves Can I have some help to understand why these pipelines are failing ? 🙏 :
|
/retest |
@valAndre07 they might be "flaky" test 🙃 😓 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
/retest |
daa90a3
to
9a84f2e
Compare
A step in a task is currently represented in the UI using a field (name). That is meant to be machine readable, not human readable. A new field displayName is added to Step.
9a84f2e
to
92b782f
Compare
Changes
Before this commit, a step in a task is currently represented in the UI using a field (name) that is meant to be machine readable, not human readable. So new field are added to Step for display name.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes