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

lxc: Accept volume full name on detach #14593

Merged
merged 5 commits into from
Dec 11, 2024
Merged

lxc: Accept volume full name on detach #14593

merged 5 commits into from
Dec 11, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Dec 5, 2024 •
edited
Loading

The completions suggest the volume name but the client doesn't parse it correctly and this has been annoying me, so I took some of my time off to fix it and also improved the completions for detach/attach commands. Along with other small improvements.

@hamistao hamistao force-pushed the accept_full_volume_name branch 2 times, most recently from 68ff64f to 20b1408 Compare December 6, 2024 06:25
@hamistao hamistao requested a review from kadinsayani December 6, 2024 06:30
@hamistao hamistao force-pushed the accept_full_volume_name branch 2 times, most recently from d1bcf4c to 6491103 Compare December 6, 2024 07:13
@hamistao hamistao marked this pull request as ready for review December 6, 2024 07:42
@tomponline tomponline requested a review from MggMuggins December 6, 2024 08:12
tomponline
tomponline reviewed Dec 6, 2024
View reviewed changes
lxc/storage_volume.go Outdated
@@ -323,26 +324,26 @@ func (c *cmdStorageVolumeAttachProfile) run(cmd *cobra.Command, args []string) e
return errors.New(i18n.G("Missing pool name"))
}

volName, volType := parseVolume("custom", args[1])
if volType != "custom" {
return errors.New(i18n.G("Only \"custom\" volumes can be attached to instances"))
Copy link
Member

Choose a reason for hiding this comment

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

lets use backticks here to save needing the escaping

tomponline
tomponline reviewed Dec 6, 2024
View reviewed changes
lxc/storage_volume.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the accept_full_volume_name branch 2 times, most recently from 6201f0c to f73656e Compare December 6, 2024 08:30
@tomponline tomponline marked this pull request as draft December 6, 2024 12:58
@tomponline
Copy link
Member

tests failing, converted to draft

@kadinsayani
Copy link
Contributor

Can we please update cmpStoragePoolWithVolume and references to it as well?

kadinsayani
kadinsayani reviewed Dec 6, 2024
View reviewed changes
lxc/completion.go Outdated Show resolved Hide resolved
kadinsayani
kadinsayani reviewed Dec 6, 2024
View reviewed changes
lxc/storage_volume.go Outdated Show resolved Hide resolved
kadinsayani
kadinsayani previously requested changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@kadinsayani kadinsayani left a comment

Choose a reason for hiding this comment

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

Just a few small nits - thanks :)

@hamistao
Copy link
Contributor Author

hamistao commented Dec 6, 2024

Can we please update cmpStoragePoolWithVolume and references to it as well?

@kadinsayani Sorry I am confused by this, why would it need ot be updated on this PR?

@kadinsayani
Copy link
Contributor

Can we please update cmpStoragePoolWithVolume and references to it as well?

@kadinsayani Sorry I am confused by this, why would it need ot be updated on this PR?

Sorry, I was confused by the function naming - we don't need to update cmpStoragePoolWithVolume.

@hamistao hamistao force-pushed the accept_full_volume_name branch 2 times, most recently from 7f530e3 to 8b3dcca Compare December 6, 2024 19:49
@hamistao
Copy link
Contributor Author

hamistao commented Dec 6, 2024 •
edited
Loading

@MggMuggins I happened to parse the volume name on detach similarly to what you have done on #14532. Since this PR is pretty simple and straightforward overall, I suggest merging this as is and you can soon rebase your PR, adding your changes on top of mine, what do you think?

@hamistao hamistao force-pushed the accept_full_volume_name branch from 8b3dcca to b54be69 Compare December 9, 2024 04:23
MggMuggins
MggMuggins previously requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@MggMuggins MggMuggins left a comment

Choose a reason for hiding this comment

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

With the exception of 8128f85, I am happy to rebase over this.

lxc/storage_volume.go Outdated Show resolved Hide resolved
lxc/completion.go Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the accept_full_volume_name branch from b54be69 to 616c11c Compare December 10, 2024 02:29
@hamistao hamistao marked this pull request as ready for review December 10, 2024 05:17
@hamistao
Copy link
Contributor Author

All comments were addressed @kadinsayani @MggMuggins @tomponline

tomponline
tomponline reviewed Dec 10, 2024
View reviewed changes
test/suites/storage.sh Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the accept_full_volume_name branch 2 times, most recently from 65777f4 to 29c54c2 Compare December 10, 2024 12:16
@hamistao
lxc/storage_volume: Accept volume full name on detach
d7d6863 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxc/storage_volume: Add comment on path argument
9a3ff88 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxc/completions: Allow filtering volume completions by type
d007e52 
Signed-off-by: hamistao <[email protected]>
@hamistao
lxc: Update cmpStoragePoolVolumes usage
bfa0c62 
On attach and detach commands, only include custom volumes as those are
the only ones attachable.

Signed-off-by: hamistao <[email protected]>
@hamistao
test: Improve attach/detach tests
2ef5355 
This checks the device entry on `lxc config show`, checks that the entry
is gone after detaching and tests detaching with the full volume name,
e.g. custom/c1pool1.

Signed-off-by: hamistao <[email protected]>
@hamistao hamistao force-pushed the accept_full_volume_name branch from 29c54c2 to 2ef5355 Compare December 10, 2024 19:45
tomponline
tomponline approved these changes Dec 11, 2024
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!

@tomponline tomponline dismissed stale reviews from MggMuggins and kadinsayani December 11, 2024 09:51

fixed

@tomponline
Copy link
Member

@MggMuggins please feel free to make changes to this in your later PR that feel appropriate when being able to attached root disks too. Ta

@tomponline tomponline merged commit edcc404 into canonical:main Dec 11, 2024
27 checks passed
@hamistao hamistao deleted the accept_full_volume_name branch December 11, 2024 18:43
@hamistao hamistao mentioned this pull request Dec 12, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MggMuggins MggMuggins MggMuggins left review comments

@kadinsayani kadinsayani kadinsayani left review comments

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamistao @tomponline @kadinsayani @MggMuggins