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.

Sign up for GitHub

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

Jump to bottom

Use MarkStatus* helpers in pkg/pod/status #2804

Merged

tekton-robot merged 1 commit into tektoncd:master from adshmh:2770-MarkStatus-helpers-for-pod-status

Jul 16, 2020

Contributor

adshmh commented Jun 10, 2020

Changes

Use MarkStatus* helper functions in pkg/pod/status.go
Closes #2770

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

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

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

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.

Release Notes

tekton-robot requested review from dlorenc and a user

June 10, 2020 19:51

tekton-robot added the size/M label

Collaborator

tekton-robot commented Jun 10, 2020

Hi @adshmh. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tekton-robot added the needs-ok-to-test label

Collaborator

tekton-robot commented Jun 10, 2020

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.

2 similar comments

Collaborator

tekton-robot commented Jun 10, 2020

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.

Collaborator

tekton-robot commented Jun 10, 2020

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.

Member

dibyom commented Jun 10, 2020

/ok-to-test

tekton-robot added ok-to-test and removed needs-ok-to-test labels

Collaborator

tekton-robot commented Jun 10, 2020

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.

1 similar comment

Collaborator

tekton-robot commented Jun 10, 2020

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.

Collaborator

tekton-robot commented Jun 10, 2020

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/pod/status.go	88.4%	89.0%	0.6

Contributor Author

adshmh commented Jun 10, 2020

/retest

vdemeester reviewed

View reviewed changes

Member

vdemeester left a comment

/kind cleanup

pkg/pod/status.go Outdated

-              			Reason:  v1beta1.TaskRunReasonRunning.String(),
-              			Message: "Not all Steps in the Task have finished executing",
-              		})
+              		MarkStatusRunning(trs, "", "")

Member

vdemeester Jun 11, 2020

We need to keep the message, don't we ?

Member

afrittoli Jun 11, 2020

And the reason!

Contributor Author

adshmh Jun 11, 2020

Thank you for the review. Fixed.

pkg/pod/status.go Outdated

-              			Reason:  v1beta1.TaskRunReasonRunning.String(),
-              			Message: "Not all Steps in the Task have finished executing",
-              		})
+              		MarkStatusRunning(trs, "", "")

Member

vdemeester Jun 11, 2020

Same here

Contributor Author

adshmh Jun 11, 2020

Thank you for the review. Fixed.

tekton-robot added kind/cleanup labels

afrittoli reviewed

View reviewed changes

Member

afrittoli left a comment

Thanks for working on this!
I agree we need to keep reason and message.

It seems worrying that all test pass with them missing, sounds like we lack some unit test coverage.
As part of this, could you add some unit test coverage?

adshmh force-pushed the 2770-MarkStatus-helpers-for-pod-status branch from 3ded563 to ed48053 Compare

June 11, 2020 20:47

Contributor Author

adshmh commented Jun 11, 2020

Thanks for working on this!
I agree we need to keep reason and message.

It seems worrying that all test pass with them missing, sounds like we lack some unit test coverage.
As part of this, could you add some unit test coverage?

Thank you for the review. I reviewed the code again, the tests were passing because I was setting default values for Reason and Message fields in the helper function. I have updated the helper function's signature to fix this which was causing confusion. The tests seem to cover all cases involving helper functions.

Collaborator

tekton-robot commented Jun 11, 2020

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/pod/status.go	88.4%	88.7%	0.2

bobcatfish reviewed

View reviewed changes

Collaborator

bobcatfish left a comment

Could we add some unit tests for the new functions as well?

pkg/pod/status.go

@@ @@ -359,6 +334,33 @@ func getWaitingMessage(pod *corev1.Pod) string { @@
               	return "Pending"
               }
+              func MarkStatusRunning(trs *v1beta1.TaskRunStatus, reason, message string) {

Collaborator

bobcatfish Jun 18, 2020

as public exported functions, the convention is to include docstrings

Contributor Author

adshmh Jun 19, 2020

Thank you for the review. Fixed.


          Use MarkStatus* helpers in pkg/pod/status

3fb974e

Use MarkStatus* helper functions in pkg/pod/status.go
Closes tektoncd#2770

adshmh force-pushed the 2770-MarkStatus-helpers-for-pod-status branch from ed48053 to 3fb974e Compare

June 19, 2020 19:07

tekton-robot added size/L and removed size/M labels

Collaborator

tekton-robot commented Jun 19, 2020

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/pod/status.go	86.7%	87.0%	0.3

vdemeester reviewed

View reviewed changes

Member

vdemeester left a comment

/lgtm
/cc @afrittoli

tekton-robot requested a review from afrittoli

July 6, 2020 08:54

tekton-robot assigned vdemeester

tekton-robot added the lgtm label

Member

pritidesai commented Jul 7, 2020

This looks good and as an additional effort/PR, we could update many unit tests from the same test file status_test.go to use these helpers, e.g. running condition here, failed condition here, etc instead of creating apis.Condition over and over.

/lgtm

tekton-robot assigned pritidesai

Contributor

dlorenc commented Jul 16, 2020

/approve nice!

Collaborator

tekton-robot commented Jul 16, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc

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:

~~OWNERS~~ [dlorenc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added the approved label

tekton-robot merged commit 76aff13 into tektoncd:master

adshmh mentioned this pull request

use status helper functions in pod status unit tests #3141

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved kind/cleanup lgtm ok-to-test size/L