-
Notifications
You must be signed in to change notification settings - Fork 4
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
Automatic Go workspace initialization #383
base: master
Are you sure you want to change the base?
Conversation
6b0c555
to
e89a429
Compare
@@ -145,12 +146,50 @@ export function bridgedProvider(config: BridgedConfig): Makefile { | |||
], | |||
], | |||
}; | |||
|
|||
workspace = { |
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 having way too much fun here with meta-make.
ifeq ($(GOWORK),off) | ||
@echo "# Creating a release build. For local development, unset the GOWORK environment variable." | ||
else | ||
@echo "# ⚠️ Creating a local dev build. For release builds prior to making a pull request, set GOWORK=off." |
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.
Thanks for clarifying the docs here.
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 would like to hold this for discussion with the team.
What problem does this address? I have had few good experiences with Go workspaces, and I would prefer this not be part of the default, as I doubt I'm the only one who is not using Go workspaces at the moment.
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.
As noted on the slack thread I think this is the wrong approach. I think this adds a lots of complexity to our build processes which could make things more difficult to diagnose when something goes wrongs and harder to make structural changes to our providers and builds.
A better approach would be to have
- A standalone target to set up the workspace if you want it.
- Set
GOWORK=off
by default when running through the makefile so we have consistency between local and CI builds. - Add an environment to not disable workspaces in the makefile.
I think this should go to ideation before opening a PR.
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.
The code change looks good. The review experience is impressively bad. We should consider changing how this repo works so we don't need to check in every file.
I agree with the team that this should probably go through an ideation, given the current lack of buy-in.
Echoing Ian. Makefile target looks fine; will give this another look once there's agreement on the approach. |
@danielrbradley @guineveresaenger Happy to go through ideation, but I would be unhappy to delay this and do not think we can afford to have our contributor & employee onboarding UX have these outcomes. I cloned a repository,
|
@AaronFriel I'm really concerned by all the ❌s next to |
Yes, if a user clones a repository into a directory, and that directory has a parent Go workspace, the outcomes are especially bad. Until the user adds the repository to their workspace, they will see some potentially misleading error messages. The error that tells the user to |
Configures Go workspaces in bridged provider repositories.
Users will experience Go workspaces configured on their behalf, unless they have opted out of Go workspaces. CI sets
GOWORK=off
to opt out in all of the workflows related to build and test.Builds with
GOWORK=off
are considered release builds in the updated Makefile and contributing guide.See this PR and associated branch for a demonstration: