- Notifications
You must be signed in to change notification settings - Fork928
feat(cli): add--provisioner-log-debug option#14558
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
Conversation
cli/parameter.go Outdated
| provisioner bool | ||
| } | ||
| func (bdf *buildDebugFlags) cliOptions() []serpent.Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we instead hook into theCODER_VERBOSE setting?
I'm curious why we need a new setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Enabling provisioner debug logs is only allowed if--enable-terraform-debug-mode is set. Hooking intoCODER_VERBOSE would potentially make unrelated stuff fail if this was not set.
If we did want to re-use this flag, we would need to first fetch the deployment config and check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not a fan of piggy-backing on verbose either, it essentially changes the behavior of a build, not just the output of the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the flag naming could be a bit better, but otherwise LGTM.
cli/parameter.go Outdated
| func (bdf *buildDebugFlags) cliOptions() []serpent.Option { | ||
| return []serpent.Option{ | ||
| { | ||
| Flag: "debug-provisioner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're not so much debugging the provisioner as we're debugging the build via increased provisioner logs. I think we can do a bit better on the naming but I don't have any great ideas. Maybe--debug-build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I could just straight-up make it--provisioner-log-debug? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That still sounds like provisioner debugging IMO, not build debugging 😅
| @@ -8,6 +8,11 @@ USAGE: | |||
| Aliases: rm | |||
| OPTIONS: | |||
| --debug-provisioner bool | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we keep this flag hidden? I believe this is only for template admins as it may leak some secrets in the TF verbose mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm happy to hide it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Stamp 👍
--provisioner-log-debug optionbcf9bc3 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Allows starting a build in debug mode from the CLI without having to have the build fail first by adding
--provisioner-log-debug.