Skip to content

[Refactor] Refactor all the GlobalCommandOptions from pointer to value reference#1824

Merged
AkihiroSuda merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/refactor-GlobalCommandOptions
Jan 11, 2023
Merged

[Refactor] Refactor all the GlobalCommandOptions from pointer to value reference#1824
AkihiroSuda merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/refactor-GlobalCommandOptions

Conversation

@Zheaoli
Copy link
Copy Markdown
Member

@Zheaoli Zheaoli commented Jan 10, 2023

Based on the discussion #1820 (comment)

Signed-off-by: Zheao.Li me@manjusaka.me

@Zheaoli Zheaoli force-pushed the manjusaka/refactor-GlobalCommandOptions branch from d24973b to d55d655 Compare January 10, 2023 10:17
Copy link
Copy Markdown
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left some minor comments. thanks

Comment thread cmd/nerdctl/volume.go
Comment on lines +47 to 49
func getVolumeStore(globalOptions types.GlobalCommandOptions) (volumestore.VolumeStore, error) {
return volume.Store(globalOptions.Namespace, globalOptions.DataRoot, globalOptions.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.

I suggest we remove this function and use volume.Store (the 2nd one below) directly (we can change volume.Store to accept a globalOptions) (Don't necessarily be part of this PR though)

This adds one unnecessary wrap on volume create:

func getVolumeStore(globalOptions *types.GlobalCommandOptions) (volumestore.VolumeStore, error) {
return volume.Store(globalOptions.Namespace, globalOptions.DataRoot, globalOptions.Address)
}

func Store(ns string, dataRoot string, address string) (volumestore.VolumeStore, error) {
dataStore, err := clientutil.DataStore(dataRoot, address)
if err != nil {
return nil, err
}
return volumestore.New(dataStore, ns)
}

func New(dataStore, ns string) (VolumeStore, error) {

Copy link
Copy Markdown
Member Author

@Zheaoli Zheaoli Jan 10, 2023

Choose a reason for hiding this comment

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

This should be another PR, I will update ASAP

Comment thread cmd/nerdctl/run.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-GlobalCommandOptions branch from d55d655 to be4beca Compare January 10, 2023 15:54
@Zheaoli Zheaoli requested a review from djdongjin January 10, 2023 15:55
Copy link
Copy Markdown
Member

@djdongjin djdongjin 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. thanks

@Zheaoli Zheaoli force-pushed the manjusaka/refactor-GlobalCommandOptions branch from be4beca to 8b60831 Compare January 10, 2023 16:51
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-GlobalCommandOptions branch 2 times, most recently from 3be396f to 90eedf0 Compare January 11, 2023 09:07
…e reference

Signed-off-by: Zheao.Li <me@manjusaka.me>
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-GlobalCommandOptions branch from 90eedf0 to f54251c Compare January 11, 2023 09:14
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 11, 2023
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 9d7833a into containerd:main Jan 11, 2023
@Zheaoli Zheaoli deleted the manjusaka/refactor-GlobalCommandOptions branch January 11, 2023 12:57
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.

3 participants