-
Notifications
You must be signed in to change notification settings - Fork 221
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
TEP-0047: Pipeline Task Display Name #319
TEP-0047: Pipeline Task Display Name #319
Conversation
|
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.
/kind tep
thanks @itewk for the all the updates, looks good to me. You will have to squash all the commits into one and pick up a TEP number based on the instructions here before we can merge this. Also, just spelling it out, this TEP is proposing an API change in |
42a1e82
to
19a7a12
Compare
19a7a12
to
cc147ac
Compare
@pritidesai done |
thanks 🙏 /lgtm (we already have one more TEP with 0046 #318 😄 ). You might have to bump your TEP number. |
@pritidesai should i bump it now? i had used the script to have it calculate the number, but i suppose that doesn't take into account other in progress PRs. |
yes please if you can, that would be great 🙏 |
/lgtm |
cc147ac
to
2356c19
Compare
@pritidesai updated the TEP number as requested and re-based |
I think this sounds like a pro in favor of Possible solution 1? - Suggest adding something like "since we have established fields like And also possibly a con against Possible solution 2? - Suggest adding something like "moving to annotations may mean we need to deprecate the existing description fields like We'll have to do some legwork here when taking the TEP to |
@sbwsg @AlanGreene sooo....should I be updating the TEP in some way in order to get it merged in? |
5c1ce59
to
1b682b8
Compare
rebased on latest from main |
Probably the quickest way to get this merged would be to remove any mention of implemention. Remove possible solution 1, possible solution 2. Remove mention of specific field names such as |
(But even after making those changes you'll still need two reviewers to agree that the TEP is mergeable. I'm just saying what I would lgtm from my perspective. Another reviewer may see things differently.) |
@sbwsg @AlanGreene @chmouel @pritidesai i'm confused, because the template explicitly calls out putting in suggestions for implementation, isn't that what they are exactly, just suggestions, and then whoever implements can do whatever ends up making sense? If you all want me to remove the suggestions I will. It seems everyone agrees this feature is needed, however its implemented, I am looking to the smoothest path to at least have it on the road map. |
1b682b8
to
55de2ff
Compare
Right, this is what I'm suggesting we reduce the TEP to - the parts folks agree on. With suggested implementations, as reviewers we have to figure out whether they're workable and what trade-offs we make in the system's design as a result. It looks like most of the sticking points here are centered on those suggestions, hence my proposal to remove them entirely. Again, other folks may feel differently and I'd like to hear from them because my read of all this could be entirely off base. |
@sbwsg understood. @AlanGreene @chmouel @pritidesai what are your all thoughts? should i remove any suggestion for specific implementation to be able to merge, or are you all ready to review and merge as is? |
We all seem to agree this is something that's needed / worth doing, but there's still some work to be done around the proposed implementation. I'd be on board with @sbwsg's suggestion to remove the implementation details for now to get the TEP merged. |
55de2ff
to
abd66ca
Compare
abd66ca
to
fae0d6f
Compare
@sbwsg @AlanGreene removed any implementation suggestions and re-based. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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 |
@AlanGreene @pritidesai this TEP looks ready to merge to my 👀 . What do you folks think? |
/lgtm |
wahooo!!! thanks everyone |
adding a new TEP for Pipeline task display name.