-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add pipeline results #2178
Add pipeline results #2178
Conversation
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
I rebased on master. |
The following is the coverage report on pkg/.
|
Apologies, I haven't had time to review much the past week or two due to the Beta release coming up. I'm still quite focused on that work so can't commit to reviewing right now. I will try to carve out some time next week (when I am on buildcop rotation) to pick up reviews again. Sorry for the frustrating lack of feedback so far! |
The following is the coverage report on pkg/.
|
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.
Really sorry for the delay in reviewing! I've left a bunch of comments but I think the overall approach here is a good way to go.
It would be amazing if we could pull all of the ResultRef-related processing out of the v1beta1 package completely. It lives right now in param_types.go but I feel like it's quickly growing out of the scope of that file. Maybe we could put it in its own package like pkg/result
similar to pkg/workspace
? Or should we tackle that in a future refactoring PR?
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
/test pull-tekton-pipeline-integration-tests |
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
@sbwsg I think I addressed all your concerns. Could you please review the latest input? Thank you. |
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.
/meow
In response to this:
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/test-infra repository. |
[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 |
if ok { | ||
got, err := v1beta1.NewResultRefs(expressions) | ||
if tt.wantErr != (err != nil) { | ||
t.Errorf("TestNewResultReference/%s wantErr %v, error = %v", tt.name, tt.wantErr, err) |
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.
Now that we have resultref.go we should also move these tests into resultref_test.go:
- TestNewResultReference
- TestHasResultReference
- TestLooksLikeResultRef
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.
ok, will do.
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.
If I change that, a new approval will be needed.
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
/lgtm |
🎉 |
Hey @othomann @vdemeester @sbwsg I'm having trouble finding an issue and/or use cases associated with this feature, can you point me in the right direction? |
Here's the issue: #1950 |
Changes
Add pipeline results to the pipeline spec and pipeline run status.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Release Notes