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

Autogenerate docs for all envbuilder flags #130

Closed
Tracked by #132
bpmct opened this issue Apr 18, 2024 · 10 comments · Fixed by #147
Closed
Tracked by #132

Autogenerate docs for all envbuilder flags #130

bpmct opened this issue Apr 18, 2024 · 10 comments · Fixed by #147
Assignees

Comments

@bpmct
Copy link
Member

bpmct commented Apr 18, 2024

There are many flags in envbuilder.go that are not documented in the README. We should document this in a README or another place and make it auto-generated

@BrunoQuaresma
Copy link
Contributor

@mtojek my plans for this task:

  • Create a go script in /scripts/doc_envbuilder_flags.go that adds all the flags to the README.
  • I'm wondering if tests can be ignored here since it is not a critical flow.
  • Add a step in the CI to check if the README is updated with the latest flags.

Do you think these steps make sense?

@mtojek
Copy link
Member

mtojek commented Apr 22, 2024

Sounds good to me! Only few comments:

/scripts/doc_envbuilder_flags.go - maybe go with a dedicated directory in case we add more scripts with multiple files, so /scripts/docsgen/main.go

I'm wondering if tests can be ignored here since it is not a critical flow.

Yeah I would assume so as in this case results, generated docs, could be reviewed manually in PRs.

Add a step in the CI to check if the README is updated with the latest flags.

Yes, must have.

@BrunoQuaresma
Copy link
Contributor

The current Options structure looks like this:

type Options struct {
	// SetupScript is ran as the root user prior to the init script.
	// It is used to configure envbuilder dynamically during the runtime.
	// e.g. specifying whether to start `systemd` or `tiny init` for PID 1.
	SetupScript string `env:"SETUP_SCRIPT"`
}

From this current structure I can get the flag name: SETUP_SCRIPT and the type: string but I can't get the description as it is a comment. At first, I can see two options:

1. Create a tag called desc and add it to the option.

type Options struct {
	SetupScript string `env:"SETUP_SCRIPT" desc:"SetupScript is ran as the root user prior to the init script. It is used to configure envbuilder dynamically during the runtime. e.g. specifying whether to start 'systemd' or 'tiny init' for PID 1."`
}

This option doesn't require major code changes, but having the description as a single-line string can be difficult to read.

2. Create another type to encapsulate each option

type Option struct {
	Env string
	Description string
	Type interface{}
}

type Options struct {
	SetupScript Option{
		Env: "SETUP_SCRIPT",
		Description:  `
			SetupScript is ran as the root user prior to the init script.
			It is used to configure envbuilder dynamically during the runtime.
			e.g. specifying whether to start 'systemd' or 'tiny init' for PID 1
		`,
		Type: string,
	}
}

This would require extra changes to the envbuilder module, which in itself isn't a bad thing, but I want to avoid adding unnecessary complexity.

@mtojek thoughts?

@mtojek
Copy link
Member

mtojek commented Apr 22, 2024

Ammar created coder/serpent to build nice CLI interfaces. If you need to adjust structures, I would recommend migrating to this project, and...

that matches solution no 2. with a separate property for description. Maybe consider building the docsgen for serpent as a new OSS project?

@BrunoQuaresma
Copy link
Contributor

Ammar created coder/serpent to build nice CLI interfaces. If you need to adjust structures, I would recommend migrating to this project, and...

I like this idea 👍

that matches solution no 2. with a separate property for description. Maybe consider building the docsgen for serpent as a new OSS project?

I like this idea too, but I would make it a separate issue/milestone.

@BrunoQuaresma
Copy link
Contributor

@mtojek taking a closer look at the envbuilder code, it looks like envbuilder was not intended to be a CLI 😅 so it might make sense to update its types internally.

@mtojek
Copy link
Member

mtojek commented Apr 22, 2024

Technically yes, but you can map ENV variables to CLI flags 👍

@mafredri
Copy link
Member

I'd be in favor of allowing all envbuilder settings to be adjusted with both flags and environment variables. This may also help in testing envbuilder functionality.

Also, if you're looking to parse Go source files @BrunoQuaresma, I'd recommend taking a look at: https://github.com/dave/dst. Utilizing that, you can get comments as well.

@mtojek
Copy link
Member

mtojek commented Apr 22, 2024

I'd be in favor of allowing all envbuilder settings to be adjusted with both flags and environment variables. This may also help in testing envbuilder functionality.

This would be much simpler :)

Also, if you're looking to parse Go source files @BrunoQuaresma, I'd recommend taking a look at: https://github.com/dave/dst.

Please nooo ... :D

@BrunoQuaresma
Copy link
Contributor

Ok, after some thought I'm going to play around with the idea of transforming envbuilder into a CLI using coder/serpent.

I will share my progress here over the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants