Skip to content

Refactor the volume ls command flagging process#1773

Merged
AkihiroSuda merged 1 commit into
containerd:mainfrom
miles170:issue-1680-refactor-volume-ls
Jan 2, 2023
Merged

Refactor the volume ls command flagging process#1773
AkihiroSuda merged 1 commit into
containerd:mainfrom
miles170:issue-1680-refactor-volume-ls

Conversation

@miles170
Copy link
Copy Markdown
Contributor

@miles170 miles170 commented Dec 27, 2022

See #1680

  • Create a file in pkg/api/types/${cmd}_types.go, and define the CommandOption for this command
  • Create some file in pkg/api/cmd/${cmd}, and move the command entry point in real into this package

Please let me know if I'm doing it incorrectly.

Comment thread docs/reference/api/volume_ls_types.md Outdated
Comment thread pkg/api/cmd/volume_ls.go Outdated
@miles170 miles170 changed the title Refactor the volume ls command flagging process Refactor the volume command flagging process Dec 27, 2022
@miles170 miles170 changed the title Refactor the volume command flagging process Refactor the volume flagging process Dec 27, 2022
@miles170 miles170 marked this pull request as draft December 27, 2022 15:03
@miles170 miles170 changed the title Refactor the volume flagging process Refactor the volume ls command flagging process Dec 27, 2022
@miles170 miles170 force-pushed the issue-1680-refactor-volume-ls branch from 7d8aaa7 to 413cf25 Compare December 27, 2022 15:50
@miles170 miles170 marked this pull request as ready for review December 27, 2022 15:50
@miles170 miles170 force-pushed the issue-1680-refactor-volume-ls branch from 413cf25 to fda89c9 Compare December 27, 2022 15:51
@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Dec 27, 2022

This PR should be merged after #1774

@AkihiroSuda AkihiroSuda marked this pull request as draft December 28, 2022 00:27
@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Dec 31, 2022

Since #1779 has been merged, I think this PR is ok to merge after resolve the code conflict

@miles170 miles170 force-pushed the issue-1680-refactor-volume-ls branch from fda89c9 to 4040932 Compare January 2, 2023 09:06
@miles170 miles170 marked this pull request as ready for review January 2, 2023 09:06
@miles170
Copy link
Copy Markdown
Contributor Author

miles170 commented Jan 2, 2023

Shouldn't we add a prefix to the struct inside apparmor_types.go?
I renamed the LsCommandOptions to VolumeLsCommandOptions to avoid conflict.

@miles170 miles170 force-pushed the issue-1680-refactor-volume-ls branch from 4040932 to befac3a Compare January 2, 2023 09:32
@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Jan 2, 2023

Shouldn't we add a prefix to the struct inside apparmor_types.go?

I renamed the LsCommandOptions to VolumeLsCommandOptions to avoid conflict.

Nice catch, I will add the prefix

Copy link
Copy Markdown
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM on CI green

Comment thread pkg/cmd/volume/volume.go Outdated
Comment thread pkg/cmd/volume/ls.go Outdated
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 2, 2023
Signed-off-by: Miles Liu <miles@bung.cc>
@miles170 miles170 force-pushed the issue-1680-refactor-volume-ls branch from befac3a to c79d835 Compare January 2, 2023 14:26
Comment thread cmd/nerdctl/volume_ls.go
return err
}
volumeSize, err := cmd.Flags().GetBool("size")
options.Quiet = quiet
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: instead of storing in local variables everytime, you can do

options.Quiet, err := cmd.Flags().GetBool("quiet")
...

same for other options.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

another style I recommand is first storing all flags in local variables and at the end, create the option struct

format, err := cmd.Flags().GetBool("format")
...
quiet,  err := cmd.Flags().GetBool("quiet")
...

option := types.VolumeLsCommandOptions{
    Format: format,
    Quiet: quiet,
}

Comment thread cmd/nerdctl/volume.go
return nil, err
}
return volumestore.New(dataStore, ns)
return volume.Store(ns, dataRoot, address)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems this getVolumeStore is no longer needed as volume.Store should be preferred.

Comment thread cmd/nerdctl/volume_ls.go
return volume.Ls(options)
}

func getVolumes(cmd *cobra.Command) (map[string]native.Volume, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this getVolumes func still being used somewhere? This func seems no longer necessary since it only read 4 flags from cmd and call another func.

If it's not common enough (e.g., used a lot by other commands as well), then we should remove this helper and instead prefer calling volume.Volumes directly.

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 89f10d8 into containerd:main Jan 2, 2023
@miles170 miles170 deleted the issue-1680-refactor-volume-ls branch January 3, 2023 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants