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

system tests: new rm, build tests #6679

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

edsantiago
Copy link
Member

  • rm: confirm 'rm' and 'rm -f' on running container

  • build: shotgun test of workdir, cmd, env, labels

Signed-off-by: Ed Santiago [email protected]

@QiWang19
Copy link
Contributor

LGTM. May need a rebase to get CI regression fixed.

@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2020

LGTM
Rebase and we can get this merged, once the tests pass.

@edsantiago edsantiago force-pushed the bats branch 2 times, most recently from 41e80ac to a4576eb Compare June 22, 2020 20:46
 - rm: confirm 'rm' and 'rm -f' on running container

 - build: shotgun test of workdir, cmd, env, labels

The new build test cd's to a temporary directory, which broke
test invocations using a relative path (./bin/podman). Added
code to detect relative paths and convert them to absolute.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago
Copy link
Member Author

Tests are green. Changes since first submission for review:

  • helpers.bash - added code to convert ./bin/podman to absolute path. This is necessary because one test (and I have more in mind) currently cd's into a temporary directory to test build with relative paths. It has the unfortunate side effect of making the podman invocation line unreadable; I will submit a subsequent PR to the log formatter to address this.
  • build.bats - prepend 'l' (the letter ell) to the label name, to work around a limitation in jq wherein .foo.1bar (JSON string beginning with a digit) causes jq to choke. (The label is a random string)

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020
@rhatdan
Copy link
Member

rhatdan commented Jun 23, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan

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 Jun 23, 2020
@openshift-merge-robot openshift-merge-robot merged commit bbaba9f into containers:master Jun 23, 2020
edsantiago added a commit to edsantiago/libpod that referenced this pull request Jun 23, 2020
Reversion of one part of containers#6679: my handling of 'realpath'
would not work when $PODMAN is 'podman-remote --url etc'.
Trying to handle that case got unmaintainable; so instead
let's just force 'make {local,remote}system' to invoke
with a full PODMAN path. This breaks down if someone
runs the tests with a manual 'bats' invocation, but I
think I'm the only one who ever does that.

Since podman path will now be very long in the logs,
add code to logformatter to abbreviate it like we do
for the ginkgo logs.

And, one thing that has bugged me for a long time:
in the error logs, show a different prompt ('#' vs '$')
to distinguish root vs rootless. This should make it
much easier to see at-a-glance whether a log file
is root or not. Add tests for it.

Signed-off-by: Ed Santiago <[email protected]>
vrothberg pushed a commit to vrothberg/libpod that referenced this pull request Aug 11, 2020
Reversion of one part of containers#6679: my handling of 'realpath'
would not work when $PODMAN is 'podman-remote --url etc'.
Trying to handle that case got unmaintainable; so instead
let's just force 'make {local,remote}system' to invoke
with a full PODMAN path. This breaks down if someone
runs the tests with a manual 'bats' invocation, but I
think I'm the only one who ever does that.

Since podman path will now be very long in the logs,
add code to logformatter to abbreviate it like we do
for the ginkgo logs.

And, one thing that has bugged me for a long time:
in the error logs, show a different prompt ('#' vs '$')
to distinguish root vs rootless. This should make it
much easier to see at-a-glance whether a log file
is root or not. Add tests for it.

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants