-
Notifications
You must be signed in to change notification settings - Fork 23
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
Remove lxc version checking #41
Remove lxc version checking #41
Conversation
@@ -261,6 +262,9 @@ jobs: | |||
- name: Python3 | |||
run: sudo lxc exec test-container --env DEBIAN_FRONTEND=noninteractive -- apt-get -y install -y python3 | |||
|
|||
# Install LXD inside cotnainer its missing inside container |
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.
s/cotnainer/container/
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.
It is a comment. I would leave it and it can be fixed in another commit if the rest of the patch is good.
@@ -289,5 +293,345 @@ jobs: | |||
- name: Show test directory | |||
run: bash -c "ls -ld $GITHUB_WORKSPACE/tests/*" | |||
|
|||
# nested LXD configuration | |||
- name: Init LXD inside container | |||
run: sudo lxc exec test-container -- sudo lxd init --preseed < $GITHUB_WORKSPACE/.github/resources/lxd4-init.yml |
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.
How do you know it's version 4?
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.
Probably that's the version installed from the Github actions container.
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.
Do you have the answer @AnvithLobo?
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.
oh I wrote it as a dirty test. You can use the same LXD installation logic the original test uses. I didn't bother as anything above 18.04 ubuntu will install 4.0 by default
https://github.com/andreasscherbaum/ansible-lxc-ssh/blob/master/.github/workflows/test.yml#L74-#L80
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.
The "original" test makes sure to check the version: https://github.com/andreasscherbaum/ansible-lxc-ssh/blob/master/.github/workflows/test.yml#L80
It also fails (for now) if a newer version is found: https://github.com/andreasscherbaum/ansible-lxc-ssh/blob/master/.github/workflows/test.yml#L72
The GH Actions tests run on "ubuntu-latest" which will automatically pull in newer releases, which might be version 4, or might be a higher version. My original design concept for the tests was to make sure I only test known versions and fail early if newer versions are found which need more testing.
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.
yeah it's a good idea to have version checking for tests.
So we would need a lot of duplicate code for the host and the first lxc container.
I'm not currently sure if there is any type of functions / ansible roles of sorts for github actions that would let us reuse the code.
if not might need to just write a bunch of duplicate tasks.
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.
I'm also not aware of such roles.
self.lxc_version = 1 | ||
display.vvv("LXC v1") | ||
else: | ||
raise AnsibleConnectionFailure("Cannot identify LXC version") |
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 removes a check which is there to check that the version is known, and works.
Future versions might change the behavior.
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.
Why not add checks when needed?
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.
Anyway, the check cannot go there. It needs to be fired off later, after the _init() and I'm not sure where that is.
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.
me neither ;)
Co-authored-by: David Parrish <[email protected]> Co-authored-by: AnvithLobo <[email protected]>
0a15e0c
to
90b330f
Compare
- Add additional check for custom key path Co-authored-by: David Parrish <[email protected]> Co-authored-by: AnvithLobo <[email protected]>
90b330f
to
da83393
Compare
As far as I understand the fixed version check in #38 makes this PR obsolete: we can check the version now OK, with correct user context. |
Close this PR, as discussed here. |
Removed the unnecessary LXC version checks which should fix #37.
Added more tests originally written by @AnvithLobo
Builds on PR #40