Skip to content

BATS tests - get working again#3290

Merged
openshift-merge-robot merged 2 commits into
containers:masterfrom
edsantiago:bats
Jun 11, 2019
Merged

BATS tests - get working again#3290
openshift-merge-robot merged 2 commits into
containers:masterfrom
edsantiago:bats

Conversation

@edsantiago
Copy link
Copy Markdown
Member

Various small fixes to get BATS tests working again.
Split from #2947 because that one keeps getting stalled,
and I'm hoping these separate changes get approved.

I consider these changes urgent because RHEL8 gating
tests are failing, and will fail even more if/when #2272
gets picked up and packaged for RHEL8, and I consider
it important to have clean passing tests for RHEL8.

  • info test: 'insecure registries' is gone. A recent
    commit (d1a7378) changed the format of 'podman info',
    removing the 'insecure registries' key. Deal with it.

  • info test: remove check for .host.{Conmon,OCIRuntime}.package;
    the value on f28 and f29 is 'Unknown' (instead of an NVR).
    We can live without this check.

  • 'load' test: skip when running in CI, because stdin
    is not a tty.

  • container restore: fix arg processing. Add support to migrate containers #2272 broke argument
    processing: 'podman container restore', with no args, should
    exit with 'argument required' error. Root cause is that the
    new --import option takes the place of an argument, so the
    checkAllAndLatest() call had to be changed to not exit on error.
    Workaround is (sigh) to copy/paste the skipped checkAllAndLatest()
    code, with minor tweaks to accommodate --import.

    *** FIXME FIXME FIXME! If I understand --import correctly,
    *** there should also be a check to prevent positional
    *** arguments with --import. Can someone please confirm/deny?

Signed-off-by: Ed Santiago santiago@redhat.com

@edsantiago
Copy link
Copy Markdown
Member Author

@adrianreber could you PTAL at the restore.go changes and confirm my understanding of that combination of options? Could you also PTAL at the FIXME comment in my commit message and confirm exactly how podman should behave in that situation? Thank you.

@cevich
Copy link
Copy Markdown
Member

cevich commented Jun 10, 2019

/approve

1 similar comment
@jwhonce
Copy link
Copy Markdown
Member

jwhonce commented Jun 10, 2019

/approve

@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 10, 2019
@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 10, 2019

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, edsantiago, jwhonce, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 10, 2019

Changes LGTM, though I would like some input from @adrianreber on the --import arg

Comment thread cmd/podman/restore.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Parenthesis are not required here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confirmed, fixed, tested. I will force-push the update once I hear back re: --import. Thank you.

@edsantiago
Copy link
Copy Markdown
Member Author

/hold
pending confirmation of the --import question

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2019
@edsantiago
Copy link
Copy Markdown
Member Author

No response. I'm making a judgment call and disabling use of --import with positional args. New code is a separate commit for ease of reviewing. I also rebased against latest master and verified that tests still pass.
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2019
Various small fixes to get BATS tests working again.
Split from containers#2947 because that one keeps getting stalled,
and I'm hoping these separate changes get approved.

I consider these changes urgent because RHEL8 gating
tests are failing, and will fail even more if/when containers#2272
gets picked up and packaged for RHEL8, and I consider
it important to have clean passing tests for RHEL8.

  * info test: 'insecure registries' is gone. A recent
    commit (d1a7378) changed the format of 'podman info',
    removing the 'insecure registries' key. Deal with it.

  * info test: remove check for .host.{Conmon,OCIRuntime}.package;
    the value on f28 and f29 is 'Unknown' (instead of an NVR).
    We can live without this check.

  * 'load' test: skip when running in CI, because stdin
    is not a tty.

  * container restore: fix arg processing. containers#2272 broke argument
    processing: 'podman container restore', with no args, should
    exit with 'argument required' error. Root cause is that the
    new --import option takes the place of an argument, so the
    checkAllAndLatest() call had to be changed to not exit on error.
    Workaround is (sigh) to copy/paste the skipped checkAllAndLatest()
    code, with minor tweaks to accommodate --import.

Signed-off-by: Ed Santiago <santiago@redhat.com>
I took the liberty of combining related checks together
so as to avoid a little duplication; if this is not a
Go best practice, I will revert. I also made a minor
wording adjustment to an error message for clarity.

Also: update wording of man page.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 11, 2019

New changes LGTM

@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 11, 2019

@rhatdan @baude @haircommander @jwhonce PTAL

@haircommander
Copy link
Copy Markdown
Collaborator

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2019
@openshift-merge-robot openshift-merge-robot merged commit c385f33 into containers:master Jun 11, 2019
@adrianreber
Copy link
Copy Markdown
Collaborator

Sorry, just saw this right now. I was away from a keyboard for a bit more than a week. I see that this is merged, so actually no further comment necessary. It looks correct that --import does not need positional parameters.

@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 26, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 26, 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.

8 participants