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

Always use -- with exec delimiter #12455

Merged
merged 10 commits into from
Nov 6, 2023
Merged

Always use -- with exec delimiter #12455

merged 10 commits into from
Nov 6, 2023

Conversation

simondeziel
Copy link
Member

While not strictly required, it is best practice to use it.

@github-actions github-actions bot added the Documentation Documentation needs updating label Oct 24, 2023
@simondeziel
Copy link
Member Author

I don't get why all those tests are failing because the instance can't be found.

@simondeziel simondeziel force-pushed the exec-delimiter branch 3 times, most recently from d0f3ea0 to 1b0577b Compare October 31, 2023 14:07
@simondeziel simondeziel marked this pull request as draft October 31, 2023 14:07
tomponline
tomponline requested changes Nov 4, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks for this. I agree we should do this for the bulk of the tests, but I think we should retain at least one example of the shortcut version otherwise we wont catch it if it regresses. What do you think?

@simondeziel
test/basic: always use -- with lxc exec
a1a53f9 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/basic: test with and without "--" separator
5be46de 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/clustering: always use -- with lxc exec
5f13398 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/config: always use -- with lxc exec
c5a8ce6 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/devlxd: always use -- with lxc exec
05a7d5b 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/image_acl: always use -- with lxc exec
0cfa44e 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
test/storage_snapshots: always use -- with lxc exec
b374be3 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
doc/howto/network_ovn_setup: always use -- with lxc exec
b51a853 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
doc/howto/instances_troubleshoot: always use -- with lxc exec
fdbbb50 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel
lxd/devlxd: always use -- with lxc exec
2e022ea 
Signed-off-by: Simon Deziel <[email protected]>
@simondeziel simondeziel marked this pull request as ready for review November 6, 2023 12:28
@simondeziel
Copy link
Member Author

@tomponline I added 2 test cases of execs with/without the -- part, thanks. All occurrences are now using --, exept for https://github.com/canonical/lxd/blob/main/test/suites/projects.sh#L104 which complains that c1 cannot be found if -- is used there. If you have a theory about why that weird error happens there, I'm interested in hearing about it :)

@tomponline
Copy link
Member

@tomponline I added 2 test cases of execs with/without the -- part, thanks. All occurrences are now using --, exept for https://github.com/canonical/lxd/blob/main/test/suites/projects.sh#L104 which complains that c1 cannot be found if -- is used there. If you have a theory about why that weird error happens there, I'm interested in hearing about it :)

Check if its calling the lxc() function rather than the lxc command?

@tomponline tomponline merged commit 6ea6aea into canonical:main Nov 6, 2023
26 checks passed
@simondeziel simondeziel deleted the exec-delimiter branch November 6, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simondeziel @tomponline