Skip to content

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

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

Merged
merged 22 commits into from
Apr 29, 2024

Conversation

BrunoQuaresma
Copy link
Contributor

@BrunoQuaresma BrunoQuaresma commented Apr 24, 2024

Related to #130

mtojek
mtojek previously requested changes Apr 24, 2024
Copy link
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?

Copy link
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.

Copy link
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?

@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
@BrunoQuaresma BrunoQuaresma dismissed mtojek’s stale review April 26, 2024 15:41

Marcin is going to be on PTO next week

@BrunoQuaresma
Copy link
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
Copy link
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
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