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.

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

[release-4.6] Bug 1920626: add test to verify build /run fs contents #25838

Merged
merged 2 commits into from Jan 29, 2021
Merged

[release-4.6] Bug 1920626: add test to verify build /run fs contents #25838

merged 2 commits into from Jan 29, 2021

Conversation

gabemontero
Copy link
Contributor

manual pick of #25810

see #25829 (comment) and #25829 (comment) for the "why"

@openshift-ci-robot
Copy link

@gabemontero: Bugzilla bug 1920626 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • qe_staff
  • redhat

In response to this:

[release-4.6] Bug 1920626: add test to verify build /run fs contents

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.

@gabemontero
Copy link
Contributor Author

GCE IAM create issues e2e-gcp-builds

/test e2e-gcp-builds

@gabemontero
Copy link
Contributor Author

OK e2e-gcp-builds has passed ... that is the e2e we've updated with this PR.

/bugzilla refresh

@openshift-ci-robot
Copy link

@gabemontero: Bugzilla bug 1920626 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • qe_staff
  • redhat

In response to this:

OK e2e-gcp-builds has passed ... that is the e2e we've updated with this PR.

/bugzilla refresh

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.

@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

/assign @adambkaplan

@jwforres FYI in case the window for this week is still open this PR is the new e2e's for openshift/builder#211 and openshift/builder#212 which you approved yesterday

@bparees

does anybody know why the bugzilla bot will not add the usual labels for this one ... I don't follow the message provided.

thanks

@bparees
Copy link
Contributor

bparees commented Jan 29, 2021

does anybody know why the bugzilla bot will not add the usual labels for this one

will message you on slack.

@bparees bparees added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 29, 2021
@gabemontero
Copy link
Contributor Author

OK @adambkaplan @bparees the e2e-gcp-builds with our new tests pass in 4.6 as well

please review / tag with lgtm / approve as you deem fit

the only difference from the 4.7 PR was that Clayton's image.ShellImage()is not there yet and decided not to wait. Placeholder comment is present in code.

adambkaplan
adambkaplan reviewed Jan 29, 2021
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

nits, otherwise looks good.

test/extended/builds/run_fs_verification.go
Comment on lines +115 to +118
hasRightListing := false
if strings.Contains(logs, lsRSlashRun) || strings.Contains(logs, lsRSlashRunFIPS) {
hasRightListing = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - can't this be one statement?

Suggested change
hasRightListing := false
if strings.Contains(logs, lsRSlashRun) || strings.Contains(logs, lsRSlashRunFIPS) {
hasRightListing = true
}
hasRightListing := strings.Contains(logs, lsRSlashRun) || strings.Contains(logs, lsRSlashRunFIPS)

test/extended/builds/run_fs_verification.go
spec:
runPolicy: Serial
source:
dockerfile: "FROM %s\nRUN chmod -R uga+rwx /run\nUSER 1001"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - can we use multiline YAML syntax?

Suggested change
dockerfile: "FROM %s\nRUN chmod -R uga+rwx /run\nUSER 1001"
dockerfile: |
FROM %s
RUN chmod -R uga+rwx /run
USER 1001"

test/extended/builds/run_fs_verification.go
spec:
runPolicy: Serial
source:
dockerfile: "FROM %s\nRUN ls -R /run\nUSER 1001"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - use multiline YAML

Suggested change
dockerfile: "FROM %s\nRUN ls -R /run\nUSER 1001"
dockerfile: |
FROM %s
RUN ls -R /run
USER 1001"

@adambkaplan
Copy link
Contributor

/lgtm

Since the code already merged in 4.7, can follow up with my nits later.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4d554c1 into openshift:release-4.6 Jan 29, 2021
@gabemontero gabemontero deleted the bld-pod-run-fs-e2e-46 branch January 29, 2021 19:55
@gabemontero
Copy link
Contributor Author

/cherrypick release-4.5

@openshift-cherrypick-robot
Copy link

@gabemontero: #25838 failed to apply on top of branch "release-4.5":

Applying: add test to verify build /run fs contents, verify /run writeable
Applying: make update output
Using index info to reconstruct a base tree...
M	test/extended/util/annotate/generated/zz_generated.annotations.go
Falling back to patching base and 3-way merge...
Auto-merging test/extended/util/annotate/generated/zz_generated.annotations.go
CONFLICT (content): Merge conflict in test/extended/util/annotate/generated/zz_generated.annotations.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 make update output
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.5

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.

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

@adambkaplan adambkaplan adambkaplan left review comments

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@adambkaplan adambkaplan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gabemontero @openshift-ci-robot @bparees @adambkaplan @openshift-cherrypick-robot @openshift-merge-robot