-
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 multiarch specific fixes to the pipeline tests #3107
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/kind misc |
The following is the coverage report on the affected files.
|
test/init_test.go
Outdated
@@ -50,7 +51,26 @@ func init() { | |||
flag.BoolVar(&skipRootUserTests, "skipRootUserTests", false, "Skip tests that require root user") | |||
} | |||
|
|||
func excludeForArchitecture(callerLevel int) bool { |
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.
This wasn't clear of what it was doing before I saw https://github.com/tektoncd/pipeline/pull/3107/files#diff-9efd74042908652f77e99919725cdc2dR48
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.
added comment to explain what is going on
6c12e0a
to
04fb452
Compare
The following is the coverage report on the affected files.
|
04fb452
to
d594db1
Compare
The following is the coverage report on the affected files.
|
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.
/lgtm
test/git_checkout_test.go
Outdated
if IsExcluded(t.Name()) { | ||
t.Skipf("skip for %s architecture", runtime.GOARCH) | ||
} | ||
|
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.
nit: As this is duplicated a bunch of times, this could even be a function : skipIfExcluded(t.Name())
.
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.
good point! 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.
however it will be skipIfExcluded(t)
, as we need t
to skip it
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.
true 😁
Test code has many amd64 specific images. Tests fail on non amd64 architecture because of that. This code provides one place to keep multiarch specific updates, separated for each architecture. Multiarch utils do following: - replace image names to arch specific ones whete it is applicable - skip the tests, which fail for non amd64 arch now, based on test name and architecture - skipping will be applied for e2e and examples tests Signed-off-by: Yulia Gaponenko <[email protected]>
d594db1
to
15723e4
Compare
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.
/lgtm
The following is the coverage report on the affected files.
|
/retest |
/approve |
[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:
Approvers can indicate their approval by writing |
Changes
Code to run the tests has many amd64 specific images.
Tests fail on non amd64 architecture because of that.
This code provides one place to keep multiarch
specific updates, separated for each architecture.
Multiarch utils do following:
and architecture
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:
cmd
dir, please updatethe release Task to build and release this image.
Release Notes