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

snap: add tests for multiple commands #5488

Merged
merged 4 commits into from
Apr 16, 2023
Merged

snap: add tests for multiple commands #5488

merged 4 commits into from
Apr 16, 2023

Conversation

russoz
Copy link
Collaborator

@russoz russoz commented Nov 7, 2022 •
edited
Loading

SUMMARY

Adds tests for handling multiple snaps in the same command.

ISSUE TYPE
  • Test Pull Request
COMPONENT NAME

snap

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug integration tests/integration tests tests labels Nov 7, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Nov 7, 2022
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 16, 2022
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jan 7, 2023
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 16, 2023
@felixfontein
Copy link
Collaborator

@russoz what's the state on this?

@russoz
Copy link
Collaborator Author

russoz commented Jan 18, 2023

This is out there in the cold, crying alone in the dark.

I am working on a couple of other things first, then will come back to this.

@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Apr 1, 2023
@russoz russoz changed the title [WIP] snap: add tests for multiple commands snap: add tests for multiple commands Apr 4, 2023
@russoz
Copy link
Collaborator Author

russoz commented Apr 4, 2023

Added become: true to the tests to make it easier to test locally with vagrant.

@russoz russoz requested a review from felixfontein April 4, 2023 07:22
@ansibullbot ansibullbot removed the WIP Work in progress label Apr 4, 2023
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 5, 2023
@russoz
Copy link
Collaborator Author

russoz commented Apr 5, 2023

@felixfontein I am trying to get the vagrant-based test-runs going here, and for this one (snap) I had it working by running the test straight as vagrant user, but using the become: true. I tried to do something similar to #5144 but it complained that it was missing the --allow-root parameter, because of the needs/root in aliases.

Couple of thoughts here:

  1. If we are to use needs/root, then snap should have that flag as well, because root is required to un/install snaps. Right? Or was there any particular reason not to set that flag in this test?
  2. If we used become: true instead of needs/root, wouldn't that achieve the same result using a standard construct from Ansible instead of a construct invented exclusively for the CI? I mean, the tests worked in this PR here. I feel like I am missing some part of the picture, the flag must be used for something else as well.

@felixfontein
Copy link
Collaborator

If we are to use needs/root, then snap should have that flag as well, because root is required to un/install snaps. Right? Or was there any particular reason not to set that flag in this test?

It's probably not set because it also works without it in CI... But yes, it should be set there as well.

If we used become: true instead of needs/root, wouldn't that achieve the same result using a standard construct from Ansible instead of a construct invented exclusively for the CI? I mean, the tests worked in this PR here. I feel like I am missing some part of the picture, the flag must be used for something else as well.

I'm not sure whether that works well with ansible-test. The needs/root is mainly to make sure that you don't accidentally run tests that need root on your system without virtualization.

@felixfontein felixfontein merged commit d734094 into ansible-collections:main Apr 16, 2023
@patchback
Copy link

patchback bot commented Apr 16, 2023 •
edited
Loading

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/d7340945a4a9d8a0a8133ce77084f0f3d6902c88/pr-5488

Backported as #6340

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Apr 16, 2023
@felixfontein
Copy link
Collaborator

@russoz thanks for improving the tests!

patchback bot pushed a commit that referenced this pull request Apr 16, 2023
@russoz @patchback
snap: add tests for multiple commands (#5488)
a01da75 
* snap: add tests for multiple commands

* snap: add tests + become

* remove packages again for idempotency

* roll back become=true in tests

(cherry picked from commit d734094)
@patchback patchback bot mentioned this pull request Apr 16, 2023
Merged
@russoz russoz deleted the snap-extra-tests branch April 16, 2023 11:33
felixfontein pushed a commit that referenced this pull request Apr 16, 2023
@patchback @russoz
[PR #5488/d7340945 backport][stable-6] snap: add tests for multiple c…
71975be 
…ommands (#6340)

snap: add tests for multiple commands (#5488)

* snap: add tests for multiple commands

* snap: add tests + become

* remove packages again for idempotency

* roll back become=true in tests

(cherry picked from commit d734094)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug integration tests/integration tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@russoz @felixfontein @ansibullbot