Skip to content

chore: refactor options to separate package #278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 23, 2024
Merged

chore: refactor options to separate package #278

merged 5 commits into from
Jul 23, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Jul 23, 2024 •
edited
Loading

Part of coder/internal#11
Extracts Options to its own package to support easier importing from separate repositories.

@johnstcn johnstcn self-assigned this Jul 23, 2024
mafredri
mafredri approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I would suggest skipping the pkg prefix, otherwise LGTM. 👍🏻 We already have some packages, like devcontainer that would make sense to have on the same level IMO.

@johnstcn
Copy link
Member Author

I would suggest skipping the pkg prefix, otherwise LGTM. 👍🏻 We already have some packages, like devcontainer that would make sense to have on the same level IMO.

Fair enough. I only chose /pkg/ due to it being a commonly-used prefix, but we can strip it.

@mafredri
Copy link
Member

Fair enough. I only chose /pkg/ due to it being a commonly-used prefix, but we can strip it.

True, it's not totally uncommon, but it's a bit controversial. Also considered "legacy" https://x.com/bradfitz/status/1039512487538970624 by some 😄

mtojek
mtojek reviewed Jul 23, 2024
View reviewed changes
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.

The change itself LGTM, but would like to understand the reasons behind 👍

envbuilder.go
@@ -83,68 +84,68 @@ type DockerConfig configfile.ConfigFile
// Logger is the logf to use for all operations.
// Filesystem is the filesystem to use for all operations.
// Defaults to the host filesystem.
func Run(ctx context.Context, options Options) error {
func Run(ctx context.Context, opts options.Options) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can you justify why we need this change in the PR description? Not sure if having a package just for options isn't an antipattern in Go. It smells more like Java...

Copy link
Member Author

Choose a reason for hiding this comment

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

When we import this from a separate repo I'd rather not have to import the whole of envbuilder and instead only the things we need.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is opinionated, but this could land in /pkg/api.

@johnstcn johnstcn merged commit b272a97 into main Jul 23, 2024
4 checks passed
@johnstcn johnstcn deleted the cj/refactor-opts branch July 23, 2024 10:37
johnstcn added a commit that referenced this pull request Aug 5, 2024
@johnstcn
chore: refactor options to separate package (#278)
3c4a458 
(cherry picked from commit b272a97)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtojek mtojek mtojek left review comments

@mafredri mafredri mafredri approved these changes

@dannykopping dannykopping Awaiting requested review from dannykopping

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnstcn @mafredri @mtojek