Skip to content

refactor: refactor envbuilder to use coder/serpent as CLI engine#140

Merged
BrunoQuaresma merged 22 commits intomainfrom
bq/docs
Apr 29, 2024
Merged

refactor: refactor envbuilder to use coder/serpent as CLI engine#140
BrunoQuaresma merged 22 commits intomainfrom
bq/docs

Conversation

@BrunoQuaresma
Copy link
Copy Markdown
Contributor

@BrunoQuaresma BrunoQuaresma commented Apr 24, 2024

Related to #130

mtojek
mtojek previously requested changes Apr 24, 2024
Copy link
Copy Markdown
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I think you can review clidocgen in coder/coder and try to implement something similar or extract the clidocgen to a common /library repo?

Comment thread envbuilder.go
Comment thread envbuilder.go Outdated
Copy link
Copy Markdown
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

It probably makes sense to use coder/serpent here as what I'm seeing is essentially a reimplementation of a subset of its existing functionality.

Comment thread envbuilder.go Outdated
Comment thread options_test.go Outdated
Copy link
Copy Markdown
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I believe you are heading in the right direction. Most of my comments pertain to adhering to the Go convention, but the overall concept is correct.

I suggest narrowing the scope of this PR to solely adopting coder/serpent (what you have now) and then preparing a follow-up to implement clidocgen. What do you think?

Comment thread cmd/envbuilder/main.go Outdated
Comment thread config.go Outdated
Comment thread config.go Outdated
Comment thread envbuilder.go Outdated
Comment thread envbuilder.go Outdated
Comment thread cmd/envbuilder/main.go Outdated
Comment thread config.go Outdated
Comment thread envbuilder.go Outdated
Comment thread envbuilder_internal_test.go Outdated
Comment thread envbuilder_test.go
@BrunoQuaresma BrunoQuaresma changed the title feature: automatically add flags docs into the readme refactor: refactor envbuilder to use coder/serpent as CLI engine Apr 25, 2024
@BrunoQuaresma BrunoQuaresma marked this pull request as ready for review April 25, 2024 19:04
Comment thread envbuilder.go Outdated
Comment thread options.go Outdated
Comment thread envbuilder.go
Comment thread cmd/envbuilder/main.go Outdated
Comment thread envbuilder.go Outdated
Comment thread envbuilder.go
Comment thread envbuilder.go Outdated
Comment thread envbuilder.go
Comment thread envbuilder.go Outdated
Comment thread options.go
@BrunoQuaresma BrunoQuaresma dismissed mtojek’s stale review April 26, 2024 15:41

Marcin is going to be on PTO next week

@BrunoQuaresma
Copy link
Copy Markdown
Contributor Author

BrunoQuaresma commented Apr 26, 2024

@mafredri @johnstcn @dannykopping I think this PR is in a good state to get merged. Here are some followups from the reviews:

  • Test CLI output against a golden file
. Reference - Issue
  • Test to validate that envs given previously will still produce the same options now in Serpent
. Reference - Issue
  •  Keep the comments on the struct and instead go generate the serpent descriptions from those. Reference - Issue
  • Warn if clean up does not work when removing Docker config file. Reference - Issue

I'm planning to create an issue + PR for each one of them after merging #147.

@BrunoQuaresma BrunoQuaresma self-assigned this Apr 26, 2024
Comment thread envbuilder.go Outdated
Copy link
Copy Markdown
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

:shipit:

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 this pull request may close these issues.

6 participants