Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added the condition for args [""] and no args[] #2248

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

keshavdalmia10
Copy link

Summary

Added the condition for args [""] and no args[]

Output

The CLI will display if there are no arguments.

Before

There was no difference when args[""] or arg[]

After

Now the will display when no args.

Documentation

  • Should this change be documented?
    • Yes
    • No

Related

Resolves ##1283

@keshavdalmia10 keshavdalmia10 requested review from a team as code owners August 12, 2024 18:08
@github-actions github-actions bot added this to the 0.36.0 milestone Aug 12, 2024
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Aug 12, 2024
Signed-off-by: Keshav Dalmia <[email protected]>
@natalieparellano
Copy link
Member

Apologies for the delay in getting to this one, we will look at it soon

Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

@keshavdalmia10 thank you for taking this on, and I really apologize for the long delay in getting back to you about this PR. I think this is headed in the right direction, but we still need to pass on the EnhancedProcess so that ArgsDisplay is shown to the user.

Actually, I'm not sure that we need EnhancedProcess in pkg/client/inspect_image.go. I would make the changes here:

var processesTemplate = `
{{- if .Info.Processes }}
Processes:
TYPE SHELL COMMAND ARGS WORK DIR
{{- range $_, $p := .Info.Processes }}
{{- if $p.Default }}
{{ (printf "%s %s" $p.Type "(default)") }} {{ $p.Shell }} {{ $p.Command }} {{ StringsJoin $p.Args " " }} {{ $p.WorkDir }}
{{- else }}
{{ $p.Type }} {{ $p.Shell }} {{ $p.Command }} {{ StringsJoin $p.Args " " }} {{ $p.WorkDir }}
{{- end }}
{{- end }}
{{- end }}`

My templating is a little rusty, but you could check out https://gobyexample.com/text-templates as a place to get started inserting the needed logic into the template

proc := proc
enhancedProc := EnhancedProcess{
Process: proc,
ArgsDisplay: formatArgsDisplay(proc.Args), // Utilize the new formatArgsDisplay to enhance arg visibility
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, we set this, but it is never actually used...

Comment on lines +1078 to +1085
switch args {
case `[]`:
h.AssertEq(t, info.Processes.DefaultProcess.Args, []string{})
case `[""]`:
h.AssertEq(t, info.Processes.DefaultProcess.Args, []string{""})
default:
h.AssertEq(t, info.Processes.DefaultProcess.Args, []string{"-p", "8080"})
}
Copy link
Member

Choose a reason for hiding this comment

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

This is really testing our call to dist.GetLabel(img, platform.BuildMetadataLabel, &buildMD) - we want to test something that calls EnhancedProcess.ArgsDisplay (but nothing does currently)

@natalieparellano natalieparellano modified the milestones: 0.36.0, 0.37.0 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants