NotificationsYou must be signed in to change notification settings
Fork947
Star10.5k

feat(cli): improve devcontainer support for`coder show`#18793

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

mafredri merged 8 commits intomainfromwipp

Jul 8, 2025

Merged

feat(cli): improve devcontainer support for`coder show`#18793

mafredri merged 8 commits intomainfromwipp

Jul 8, 2025

Conversation

Copy link

Member

mafredri commentedJul 8, 2025•
edited
Loading

Fixescoder/internal#747

github-actionsbot assignedmafredri

Jul 8, 2025

mafredri force-pushed thewipp branch from28e09c5 to762257bCompare

July 8, 2025 14:27

mafredri requested review fromjohnstcn,mtojek andDanielleMaywood

July 8, 2025 14:30

mafredri marked this pull request as ready for review

July 8, 2025 14:30

feat(cli): improve devcontainer support forcoder show

ea9d210

Fixescoder/internal#747

mafredri force-pushed thewipp branch from762257b toea9d210Compare

July 8, 2025 14:35

DanielleMaywood approved these changes

Jul 8, 2025

View reviewed changes

Copy link

Contributor

DanielleMaywood left a comment

Choose a reason for hiding this comment

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

Looks good to me although question around the default value used for architecture.

cli/cliui/resources.go OutdatedShow resolvedHide resolved

mafredri added2 commits

July 8, 2025 14:39

remove silly

eb04c1a

make gen

5519faa

mtojek approved these changes

Jul 8, 2025

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

Looks pretty!

johnstcn approved these changes

Jul 8, 2025

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

Nice work! Some suggestions but nothing blocking.

cli/cliui/resources.go OutdatedShow resolvedHide resolved

cli/show.go

		{
		Flag: "details",
		Description: "Show full error messages and additional details.",
		Value: serpent.BoolOf(&details),

Copy link

Member

Choose a reason for hiding this comment

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

Suggestion: explicit default value

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I added it but it changes the CLI output to show default false. Not sure I like that to be honest since it's kinda clear that that's already the case. But either way works for me.

Copy link

Member

Choose a reason for hiding this comment

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

I'm easy either way, having an explicit default is easier to read but if it negatively impacts the help output we can skip it.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

but if it negatively impacts the help output we can skip it.

wdyt?2eab390 (#18793)

Copy link

Member

Choose a reason for hiding this comment

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

It doesn't seem so bad to me

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Alright. IMO it's somewhat of a bug in serpent, doesn't really make sense to render the two cases differently and it could already inherit the default value from the current value of the variable given as pointer¯\_(ツ)_/¯.