Skip to content

[refactor] Add global flag process helper function#1797

Merged
Zheaoli merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/global-flag
Jan 6, 2023
Merged

[refactor] Add global flag process helper function#1797
Zheaoli merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/global-flag

Conversation

@Zheaoli
Copy link
Copy Markdown
Member

@Zheaoli Zheaoli commented Jan 3, 2023

I'm so sorry I have to make a huge PR again.

After #1793, we have a types.GlobalCommandOptions, and for #1792 and #1791 and many refactor PR int he future, we may need to introduce a new helper function to process the global flag.

In this PR, I introduce the new helper function and use this function to process the global flag in all subcommand.

There may be some duplicated code for some subcommand like nerdctl run, I think this should be not a problem, I will merge it when I refactor the run command,

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

@Zheaoli Zheaoli marked this pull request as draft January 3, 2023 19:45
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.

(Not request changes)

In the current PR, most commands are refactored into a half-way state (e.g., global flag is processed (sometimes, repeatedly) whenever it's needed. Ideally, it should only be processed once at the entry of the command, and let other usage use the processed struct.

As an example, getVolumeStore calls processGlobalFlags, and getComposer calls getVolumeStore, which means each compose commands call processGlobalFlags at least twice (one in getVolumeStore, one in its own command if global flags are used).

Either way, we should highlight that helper functions (e.g., getVolumeStore) should be refactored to not call processGlobalFlags directly and instead use the struct created by the command.

Comment thread cmd/nerdctl/flagutil.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/global-flag branch from 8e7c61e to 5fceed2 Compare January 4, 2023 13:51
@Zheaoli
Copy link
Copy Markdown
Member Author

Zheaoli commented Jan 4, 2023

(Not request changes)

In the current PR, most commands are refactored into a half-way state (e.g., global flag is processed (sometimes, repeatedly) whenever it's needed. Ideally, it should only be processed once at the entry of the command, and let other usage use the processed struct.

As an example, getVolumeStore calls processGlobalFlags, and getComposer calls getVolumeStore, which means each compose commands call processGlobalFlags at least twice (one in getVolumeStore, one in its own command if global flags are used).

Either way, we should highlight that helper functions (e.g., getVolumeStore) should be refactored to not call processGlobalFlags directly and instead use the struct created by the command.

nice catch. I have update all the PR to make sure call processGlobalFlags once for each command

@Zheaoli Zheaoli force-pushed the manjusaka/global-flag branch 2 times, most recently from 3e9c335 to b9b3be1 Compare January 4, 2023 14:08
@Zheaoli Zheaoli marked this pull request as ready for review January 4, 2023 17:41
@Zheaoli Zheaoli requested a review from djdongjin January 4, 2023 18:50
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 aside from the nit. thanks

Comment thread cmd/nerdctl/flagutil.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/global-flag branch from b9b3be1 to 209ba73 Compare January 5, 2023 03:25
@Zheaoli
Copy link
Copy Markdown
Member Author

Zheaoli commented Jan 5, 2023

@AkihiroSuda PTAL

@Zheaoli Zheaoli added this to the v1.2.0 milestone Jan 5, 2023
@Zheaoli Zheaoli requested a review from djdongjin January 5, 2023 10:32
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

Comment thread cmd/nerdctl/flagutil.go
Comment on lines +65 to +72
hostsDir, err := cmd.Flags().GetStringSlice("hosts-dir")
if err != nil {
return nil, err
}
experimental, err := cmd.Flags().GetBool("experimental")
if err != nil {
return nil, err
}
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.

Could we add hosts-dir and experimental flags to README? https://github.com/containerd/nerdctl#global-flags

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can add the docs in another PR.

Comment thread cmd/nerdctl/flagutil.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/global-flag branch from 209ba73 to ac8542c Compare January 6, 2023 03:36
@Zheaoli Zheaoli requested a review from ktock January 6, 2023 03:36
Comment thread cmd/nerdctl/stop.go Outdated
Signed-off-by: Zheao.Li <me@manjusaka.me>
@Zheaoli Zheaoli force-pushed the manjusaka/global-flag branch from ac8542c to dc374a5 Compare January 6, 2023 05:59
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

@Zheaoli Zheaoli merged commit c71c288 into containerd:main Jan 6, 2023
@Zheaoli Zheaoli deleted the manjusaka/global-flag branch January 6, 2023 07:36
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