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

lxc/cluster_group: Added lxc cluster group add #11980

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

taltrums
Copy link
Contributor

@taltrums taltrums commented Jul 10, 2023

Fixes #11726
Hi @monstermunchkin , Created a clean PR for LXC cluster group add. Kindly have a look.
Thanks

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.

Please can you add a test in test_clustering_groups and update https://linuxcontainers.org/lxd/docs/latest/howto/cluster_groups/ (https://github.com/canonical/lxd/blob/main/doc/howto/cluster_groups.md) with how to use the command. Thanks!

@ru-fu
Copy link
Contributor

ru-fu commented Jul 11, 2023

https://linuxcontainers.org/lxd/docs/latest/howto/cluster_groups/

Correct URL ;) : https://documentation.ubuntu.com/lxd/en/latest/howto/cluster_groups/

@taltrums
Copy link
Contributor Author

Please can you add a test in test_clustering_groups and update https://linuxcontainers.org/lxd/docs/latest/howto/cluster_groups/ (https://github.com/canonical/lxd/blob/main/doc/howto/cluster_groups.md) with how to use the command. Thanks!

Hi @tomponline for some reason I am unable to find the test_clustering_groups file. can you point it out please. I will update the doc for the command soon. Thanks

@tomponline
Copy link
Member

@taltrums taltrums closed this Jul 11, 2023
@taltrums taltrums deleted the lxc-cluster-group-add branch July 11, 2023 15:18
@taltrums taltrums restored the lxc-cluster-group-add branch July 11, 2023 15:19
@taltrums taltrums reopened this Jul 11, 2023
@taltrums
Copy link
Contributor Author

https://github.com/search?q=repo%3Acanonical%2Flxd%20test_clustering_groups&type=code

Hi @tomponline Please review the changes for tests and let me know if anything has to change, I have also updated the document. Thanks

@github-actions github-actions bot added the Documentation Documentation needs updating label Jul 11, 2023
doc/howto/cluster_groups.md Outdated Show resolved Hide resolved
doc/howto/cluster_groups.md Outdated Show resolved Hide resolved
doc/howto/cluster_groups.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks! The doc update looks good. :)

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.

LGTM! Thanks

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.

Tests are failing:

+ eval timeout --foreground 120 /home/runner/go/bin/lxc "cluster" "group" "delete" "cluster:newgroup" --verbose
+ timeout --foreground 120 /home/runner/go/bin/lxc cluster group delete cluster:newgroup --verbose
Error: Only empty cluster groups can be removed

@taltrums
Copy link
Contributor Author

Tests are failing:

+ eval timeout --foreground 120 /home/runner/go/bin/lxc "cluster" "group" "delete" "cluster:newgroup" --verbose
+ timeout --foreground 120 /home/runner/go/bin/lxc cluster group delete cluster:newgroup --verbose
Error: Only empty cluster groups can be removed

@tomponline so basically what I am thinking is I need to remove the member before deleting the cluster group?
something like this?

Screenshot-13

@monstermunchkin
Copy link
Contributor

so basically what I am thinking is I need to remove the member before deleting the cluster group?
something like this?

Yes, before removing the cluster group itself, you need to first remove all cluster group members.

@tomponline
Copy link
Member

This needs a rebase too now.

@tomponline
Copy link
Member

Please can you rebase

Signed-off-by: taltrums <mohmmad.talha@outlook.com>
Signed-off-by: taltrums <mohmmad.talha@outlook.com>

doc: Fix lxc cluster group add command

Signed-off-by: taltrums <mohmmad.talha@outlook.com>
Signed-off-by: taltrums <mohmmad.talha@outlook.com>

test: Fix test for lxc cluster group add

Signed-off-by: taltrums <mohmmad.talha@outlook.com>
Signed-off-by: taltrums <mohmmad.talha@outlook.com>
@taltrums
Copy link
Contributor Author

Hi @tomponline, I made the changes as requested, let me know if anything needs change any further. Thanks

@taltrums taltrums requested a review from tomponline July 17, 2023 12:23
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.

LGTM, thanks!

@tomponline tomponline merged commit b36fa67 into canonical:main Jul 17, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lxc cluster group add command
4 participants