Skip to content

[Refactor] Refactor the build subcommand flagging process#1792

Merged
AkihiroSuda merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/refactor-build-flag
Jan 9, 2023
Merged

[Refactor] Refactor the build subcommand flagging process#1792
AkihiroSuda merged 1 commit into
containerd:mainfrom
Zheaoli:manjusaka/refactor-build-flag

Conversation

@Zheaoli
Copy link
Copy Markdown
Member

@Zheaoli Zheaoli commented Jan 2, 2023

Checklist:

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

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

@Zheaoli Zheaoli marked this pull request as draft January 2, 2023 15:35
Comment thread .gitignore Outdated
Comment thread pkg/api/types/global.go Outdated
@Zheaoli
Copy link
Copy Markdown
Member Author

Zheaoli commented Jan 2, 2023

Should be merged since #1793

@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch from efa8da1 to baa801b Compare January 2, 2023 19:05
@Zheaoli Zheaoli marked this pull request as ready for review January 2, 2023 19:05
@AkihiroSuda AkihiroSuda requested a review from ktock January 3, 2023 10:39
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 3, 2023
Comment thread pkg/api/types/common.go Outdated
Comment thread pkg/api/types/common.go Outdated
Comment thread pkg/api/types/common.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch from baa801b to 0c47b1c Compare January 5, 2023 17:41
@Zheaoli
Copy link
Copy Markdown
Member Author

Zheaoli commented Jan 5, 2023

should be merged after #1797

@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch from 0c47b1c to 281b190 Compare January 5, 2023 17:45
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch 2 times, most recently from ad7542f to d8da63a Compare January 6, 2023 08:00
@Zheaoli
Copy link
Copy Markdown
Member Author

Zheaoli commented Jan 6, 2023

Since #1797 has been merged, I think this PR is ready to go.

cc @AkihiroSuda @djdongjin @ktock

Comment thread cmd/nerdctl/build.go
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch 3 times, most recently from 49a21e9 to cf82e2d Compare January 6, 2023 11:24
@Zheaoli Zheaoli requested a review from ktock January 6, 2023 11:25
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.

I found multiple "assign an option to a local variable (noCache := option.GOptions.NoCache) and then only use it once" pattern. We should remove those redundant assignment code unless for convenience because it's used several times.

Thanks

Comment thread pkg/api/types/common.go Outdated
Comment thread pkg/api/types/build_types.go Outdated
Comment thread pkg/cmd/build/build.go
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread cmd/nerdctl/build.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch 3 times, most recently from 2951462 to 6d4194d Compare January 6, 2023 19:53
Comment thread pkg/api/types/common.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch from 6d4194d to 35d0127 Compare January 7, 2023 11:09
@Zheaoli Zheaoli requested a review from djdongjin January 7, 2023 14:00
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch 2 times, most recently from d09f613 to 6430c94 Compare January 7, 2023 16:10
Comment thread pkg/cmd/build/build.go Outdated
Comment on lines +186 to +185
namespace := options.GOptions.Namespace
address := options.GOptions.Address
platform := options.Platform
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.

move into if output == block, they're only used in if (also suggest use options.xxx directly (same for options.BuildKitHost below) if they're not used frequently, that makes it more clear where the variable comes from)

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.

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.

options.GOptions.Namespace and options.GOptions.Adress are only used within if output==. We can move these 2 inside.

(Although personally I will use options.Platform directly in line 285... 😅 it's so far away from its definition (line 185) and it's not changed in between)

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.

done

Comment thread pkg/cmd/build/build.go Outdated
Comment thread pkg/cmd/build/build.go
file := buildkitutil.DefaultDockerfileName
if options.File != "" {
if options.File == "-" {
var err 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.

do we need this line var err error?

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.

It's a special scope trick

if we don't have var err error, the code should be

		if options.File == "-" {
			var err error
			dir, err  := buildkitutil.WriteTempDockerfile(stdin)
			if err != nil {
				return "", nil, false, "", nil, nil, err
			}
			cleanup = func() {
				os.RemoveAll(dir)
			}
		} else {
			dir, file = filepath.Split(options.File)
		}

It means that the new dir variable will be unavailable outside the condition block....

Comment thread pkg/cmd/build/build.go Outdated
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch 4 times, most recently from 9a62f11 to d3922d9 Compare January 8, 2023 09:06
@Zheaoli Zheaoli requested a review from djdongjin January 8, 2023 09:06
Signed-off-by: Zheao.Li <me@manjusaka.me>
@Zheaoli Zheaoli force-pushed the manjusaka/refactor-build-flag branch from d3922d9 to b9ce2af Compare January 8, 2023 14:19
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, thanks

@Zheaoli Zheaoli mentioned this pull request Jan 8, 2023
2 tasks
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 010d4ae into containerd:main Jan 9, 2023
@Zheaoli Zheaoli deleted the manjusaka/refactor-build-flag branch January 11, 2023 03:13
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