fix TestTaskRunStatus e2e test to run on s390x #3061

barthy1 · 2020-08-05T13:15:22Z

Changes

TestTaskRunStatus test fails for s390x architecture because busybox
image in the code is specified with concrete SHA. SHA is unique for
each image and as a result for s390x case the amd64 image with the SHA
is pulled. Test failed.

Adding s390x specific SHA to s390x build solves this problem.

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
Release notes block has been filled in or deleted (only if no user facing changes)

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

Update the TestTaskRunStatus e2e test to work on s390x architectures.

tekton-robot · 2020-08-05T13:15:36Z

Hi @barthy1. 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.

ghost · 2020-08-05T13:17:51Z

/ok-to-test

barthy1 · 2020-08-05T14:44:11Z

Hi @sbwsg
I am new in this repo. I've got several reports about failing tests for this PR. But now I can see that only check-pr-has-kind-label failed. Is it good state for PR or I need to have a look at something?

Extra question, which golang tools or built-in make targets I need to use to verify my code? I used gofmt and got failed test with gocritic :(

ghost · 2020-08-05T14:57:03Z

/kind feature

ghost · 2020-08-05T14:59:37Z

Hi @barthy1 thanks for the contribution!

It looks like all the tests are passing now and I've added a kind label. It would be great to add the following:

Update commit message and PR description to match commit message format

Add a release note to your PR description:

Update the TestTaskRunStatus e2e test to work on s390x architectures.

ghost · 2020-08-05T15:01:31Z

Also, just connecting the dots here, this looks related to #856

ghost · 2020-08-05T15:05:02Z

Ah, sorry, missed the second part of your question:

Extra question, which golang tools or built-in make targets I need to use to verify my code? I used gofmt and got failed test with gocritic :(

I'm not familiar with gocritic unfortunately. Normally I just run make fmt and make golangci-lint. @vdemeester is there a way to run gocritic using the makefile?

TestTaskRunStatus test fails for s390x architecture because busybox image in the code is specified with concrete SHA. SHA is unique for each image and as a result for s390x case the amd64 image with the SHA is pulled. Test failed. Adding s390x specific SHA to s390x build solves this problem. Signed-off-by: Yulia Gaponenko <[email protected]>

barthy1 · 2020-08-05T15:37:02Z

hey @sbwsg, thanks for the input.

I updated the PR as per your comments (hopefully)
make fmt + make golangci-lint is what I need here, thank you
fyi I've opened generic issue for future s390x fixes in pipeline repo s390x related fixes for tekton pipeline #3064. It's part of s390x activity to have Tekton running on Z.

ghost

Awesome, thanks!

tekton-robot · 2020-08-05T15:48:01Z

[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:

~~OWNERS~~ [sbwsg]

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

vdemeester · 2020-08-05T15:50:56Z

Ah, sorry, missed the second part of your question:

Extra question, which golang tools or built-in make targets I need to use to verify my code? I used gofmt and got failed test with gocritic :(

I'm not familiar with gocritic unfortunately. Normally I just run make fmt and make golangci-lint. @vdemeester is there a way to run gocritic using the makefile?

It is doable 🙃 😝

vdemeester

/lgtm

chmouel · 2020-08-06T09:06:44Z

test/taskrun_test.go

@@ -126,13 +127,17 @@ func TestTaskRunStatus(t *testing.T) {
 	taskRunName := "status-taskrun"

 	fqImageName := "busybox@sha256:895ab622e92e18d6b461d671081757af7dbaa3b00e3e28e12505af7817f73649"
+	if runtime.GOARCH == "s390x" {
+		fqImageName = "busybox@sha256:4f47c01fa91355af2865ac10fef5bf6ec9c7f42ad2321377c21e844427972977"


We have quite a lot of places that uses the busybox image around, maybe we can have a common function which could be extended easily for other architectures if needed ?

Fix is required only for places where busybox sha is specified directly. I identified 1 piece of code for e2e tests(fixed by this PR), and 2 pieces for examples.
Just busybox image is multi-arched, and expectation here, that if you have image: busybox statement it should run on all architectures without modifications.
Anyway, it's a good point for other cases (and can be applied even here), where image name for amd64 will be different to s390x or ppx64le names. Thank you for feedback, I will work on this extension.

awesome thank you! 🙇

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 5, 2020

tekton-robot requested review from imjasonh and a user August 5, 2020 13:15

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 5, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 5, 2020

barthy1 force-pushed the busybox_multi_arch branch from ee9dfee to c972c16 Compare August 5, 2020 13:50

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 5, 2020

tekton-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 5, 2020

barthy1 force-pushed the busybox_multi_arch branch from c972c16 to d6ad7d1 Compare August 5, 2020 15:25

barthy1 mentioned this pull request Aug 5, 2020

s390x related fixes for tekton pipeline #3064

Closed

16 tasks

ghost approved these changes Aug 5, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2020

vdemeester reviewed Aug 5, 2020

View reviewed changes

tekton-robot assigned vdemeester Aug 5, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2020

tekton-robot merged commit cb85f1d into tektoncd:master Aug 5, 2020

chmouel reviewed Aug 6, 2020

View reviewed changes

barthy1 mentioned this pull request Sep 30, 2020

Add barthy1 to the org tektoncd/community#219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix TestTaskRunStatus e2e test to run on s390x #3061

fix TestTaskRunStatus e2e test to run on s390x #3061

barthy1 commented Aug 5, 2020 •

edited

Loading

tekton-robot commented Aug 5, 2020

ghost commented Aug 5, 2020

barthy1 commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

barthy1 commented Aug 5, 2020 •

edited

Loading

ghost left a comment

tekton-robot commented Aug 5, 2020

vdemeester commented Aug 5, 2020

vdemeester left a comment

chmouel Aug 6, 2020

barthy1 Aug 6, 2020

chmouel Aug 6, 2020

fix TestTaskRunStatus e2e test to run on s390x #3061

fix TestTaskRunStatus e2e test to run on s390x #3061

Conversation

barthy1 commented Aug 5, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 5, 2020

ghost commented Aug 5, 2020

barthy1 commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

ghost commented Aug 5, 2020

barthy1 commented Aug 5, 2020 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 5, 2020

vdemeester commented Aug 5, 2020

vdemeester left a comment

Choose a reason for hiding this comment

chmouel Aug 6, 2020

Choose a reason for hiding this comment

barthy1 Aug 6, 2020

Choose a reason for hiding this comment

chmouel Aug 6, 2020

Choose a reason for hiding this comment

barthy1 commented Aug 5, 2020 •

edited

Loading

barthy1 commented Aug 5, 2020 •

edited

Loading