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

Sort and remove duplicate when set "taskrun.status.taskResults" #2467

Conversation

vincent-pli
Copy link
Member

@vincent-pli vincent-pli commented Apr 22, 2020

Fix issue: #2466

  1. Sort pod.containerStatuses before parse
  2. Overwrite older items

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 22, 2020
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2020
@vincent-pli
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@vdemeester
Copy link
Member

/cc @othomann
/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 22, 2020
@vincent-pli vincent-pli force-pushed the remove-duplicate-add-sort-taskrun-result branch from 016d039 to 9ba2f91 Compare April 23, 2020 02:21
Copy link
Contributor

@othomann othomann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@vincent-pli vincent-pli force-pushed the remove-duplicate-add-sort-taskrun-result branch from 9ba2f91 to be55fca Compare May 20, 2020 00:51
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 75.2% 74.6% -0.6

@vincent-pli
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@vdemeester vdemeester removed the kind/bug Categorizes issue or PR as related to a bug. label May 20, 2020
@vdemeester
Copy link
Member

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 20, 2020
@tekton-robot
Copy link
Collaborator

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

@vdemeester
Copy link
Member

/cc @sbwsg @bobcatfish

@tekton-robot tekton-robot requested review from bobcatfish and a user May 20, 2020 09:45
@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

@ghost
Copy link

ghost commented May 20, 2020

Thanks for this @vincent-pli could you remove the Release Notes section in the PR description if there's none to include?

@vincent-pli vincent-pli force-pushed the remove-duplicate-add-sort-taskrun-result branch from be55fca to 04e178d Compare May 21, 2020 01:47
@vincent-pli
Copy link
Member Author

@sbwsg removed the Release Notes, thanks

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 75.2% 76.1% 0.8

@ghost
Copy link

ghost commented May 21, 2020

Looks good, let me know if you want to make the final suggested change and then I'll approve / lgtm!

@vincent-pli vincent-pli force-pushed the remove-duplicate-add-sort-taskrun-result branch from 04e178d to a3b3e0d Compare May 22, 2020 01:24
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 75.2% 76.1% 0.8

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2020
@jlpettersson
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@jlpettersson jlpettersson removed their assignment May 27, 2020
Fix issue: tektoncd#2466
1. Sort `pod.containerStatuses` before parse
2. Overwrite older items
@vincent-pli vincent-pli force-pushed the remove-duplicate-add-sort-taskrun-result branch from a3b3e0d to 95a805c Compare May 28, 2020 09:01
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/taskrun/taskrun.go 75.2% 76.1% 0.8

@pritidesai
Copy link
Member

thanks @vincent-pli
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2020
Copy link
Member

@pritidesai pritidesai left a comment

Choose a reason for hiding this comment

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

thanks @vincent-pli

just reiterating, task results are assigned values appearing in latest Container State i.e. task result named resultNameOne is assigned resultValueThree and not resultValueOne

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: othomann, pritidesai, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot merged commit bb99bf6 into tektoncd:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants