refactor: container logs flagging process by davidhsingyuchen · Pull Request #1881 · containerd/nerdctl

davidhsingyuchen · 2023-01-19T04:51:03Z

Part of #1680

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

Notes:

Also renamed cmd/nerdctl/logs.go to cmd/nerdctl/container_logs.go like what's done in refactor: image tag flagging process #1869.

Signed-off-by: Hsing-Yu (David) Chen davidhsingyuchen@gmail.com

djdongjin · 2023-01-19T12:57:03Z

+	"github.com/spf13/cobra"
+)
+
+func newLogsCommand() *cobra.Command {


I think we can keep the file name unchanged as logs.go (and logs_test.go)

Hmm, I was following the pattern in #1869 as container logs is the canonical name of this command. @AkihiroSuda Do you know which pattern we should follow?

ops, didn't notice that. I'm okay with either one.

Currently it seems we only prefix container/image to file names if there're mutiple commands (e.g., container_inspect.go/container_prune.go v.s. image_inspect.go/image_prune.go; run.go instead of container_run.go).

But yeah adding a prefix can make those files closer :)

My thoughts:

Prefixing the files according to their canonical names may make them more organized.

If we're grouping things according to their canonical names re. things under pkg/cmd, maybe we want to do so consistently in other places.

Just my 2 cents :)

Yes, containers_logs.go is preferred as it corresponds to the canonical form nerdctl containers logs

Wondering if I should do a one-shot PR for this. Maybe after #1900 is merged? /cc @Zheaoli

Yes, @davidhsingyuchen, it's better to be merged after #1900

djdongjin · 2023-01-19T12:58:57Z

+	if err != nil {
+		return err
+	}
+	return container.Logs(cmd.Context(), options, cmd.OutOrStdout(), cmd.ErrOrStderr())


can you move the Container out from option and pass it as an argument? Container is not really an option but the target for Logs.

container.Logs(cmd.Context(), args[0], options, ...)

I see. Shall we also move Containers out of ContainerInspectCommandOptions (in another PR)?

djdongjin · 2023-01-21T17:10:36Z

+	return logsCommand
+}
+
+func processContainerLogsCommandOptions(cmd *cobra.Command) (types.ContainerLogsCommandOptions, error) {


We may need to change ContainerLogsCommandOptions to ContainerLogsOptions (e.g. from ${Command}CommandOptions to ${Command}Options) for brevity. And also put stdin/stdout/stderr in ${Command}Options directly (if they're used).

#1889 has more discussion, but the tl;dr is:

Rename ${Command}CommandOptions to ${Command}Options;

Put necessary io used by the command directly in ${Command}Options;

Argument order: func Command(ctx context.Context, param1, param2 type, options ${Command}Options (param1, param2 type means, e.g., container_id, image_ref, id, etc).

Sorry to back and forth on this, but now I think it brings more consistency :) lmk if there is something unclear.

#1900 has changes following up the issue.

Thank you for the very clear summary! Updated accordingly.

Re. #1889 (comment):

These are optional, and nil values should imply os.Stdin, os.Stdout, os.Stderr .

I didn't add a check for this as they could be nil even before they are put into the options struct, and we weren't doing the checks. Checked #1900, and it seems that it's also not adding this check. Last, currently every caller in cmd/nerdctl should just explicitly specify them anyway.

davidhsingyuchen · 2023-01-22T19:56:49Z

Forced push again to

resolve merge conflicts
move Stdout and Stderr to be consistent with what's in [Refactor] Reorder the method argument ordering in pkg/cmd #1900 (pkg/cmd: inconsistent arguments ordering #1889 (comment))

djdongjin

LGTM aside from the comment. Maybe we should merge after #1900 to reduce conflict.

djdongjin · 2023-01-22T20:16:21Z

-		return err
-	}
-	dataStore, err := clientutil.DataStore(globalOptions.DataRoot, globalOptions.Address)
+func Logs(ctx context.Context, container string, options types.ContainerLogsOptions) error {


Can you add a brief comment here?

nit: also maybe change container name to something else (for example, c, arg), since the package name is also container. Not sure if there will any unexpected conflict.

Can you add a brief comment here?

Do you mean adding a function comment for Logs? If yes, maybe we'd want to do it consistently. Currently it seems that no other function has such comment.

also maybe change container name to something else (for example, c, arg)

Maybe ctr? I feel that arg may be too generic, and c is a bit too short for a parameter.

ctr also means ctr cli from containerd, which might be confusing. I'm okay with keeping container if there is no conflict.

Rebased.

I'm okay with keeping container if there is no conflict.

Keeping it as is as there's no conflict.

davidhsingyuchen · 2023-01-22T20:26:16Z

Maybe we should merge after #1900 to reduce conflict.

Sure. Will address comments and rebase on main after #1900 is merged.

Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>

AkihiroSuda

Thanks

davidhsingyuchen marked this pull request as ready for review January 19, 2023 06:24

djdongjin reviewed Jan 19, 2023

View reviewed changes

djdongjin added the kind/refactor label Jan 19, 2023

davidhsingyuchen force-pushed the refactor-logs-flags branch from 350debf to faef906 Compare January 19, 2023 19:33

davidhsingyuchen mentioned this pull request Jan 21, 2023

refactor: container stop flagging process #1896

Merged

2 tasks

davidhsingyuchen requested a review from djdongjin January 21, 2023 01:52

djdongjin reviewed Jan 21, 2023

View reviewed changes

djdongjin mentioned this pull request Jan 21, 2023

[Refactor] refactor rmi flagging process #1892

Merged

2 tasks

davidhsingyuchen force-pushed the refactor-logs-flags branch from faef906 to 57d72cb Compare January 22, 2023 00:46

davidhsingyuchen mentioned this pull request Jan 22, 2023

refactor: pass an io.Writer to image.List #1887

Merged

davidhsingyuchen force-pushed the refactor-logs-flags branch from 57d72cb to b5c8a42 Compare January 22, 2023 19:55

djdongjin reviewed Jan 22, 2023

View reviewed changes

AkihiroSuda added this to the v1.2.0 milestone Jan 23, 2023

davidhsingyuchen force-pushed the refactor-logs-flags branch from b5c8a42 to f242365 Compare January 23, 2023 09:04

refactor: container logs flagging process

fb20478

Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>

davidhsingyuchen force-pushed the refactor-logs-flags branch from f242365 to fb20478 Compare January 23, 2023 09:13

AkihiroSuda approved these changes Jan 23, 2023

View reviewed changes

AkihiroSuda merged commit 5b84533 into containerd:main Jan 23, 2023

davidhsingyuchen deleted the refactor-logs-flags branch January 23, 2023 17:54

Conversation

davidhsingyuchen commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhsingyuchen commented Jan 22, 2023

Uh oh!

djdongjin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhsingyuchen commented Jan 22, 2023

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidhsingyuchen commented Jan 19, 2023 •

edited

Loading