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

dev/sg: add envOverrides option for sets to override command env #38318

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Jul 6, 2022

Right now, command env takes precedence over set env. I'm scared to change this, so instead I added envOverrides and prop-drilled it to override whatever env is set on the command.

Test plan

Tested in #38319 , frontend-e2e tests work now

@bobheadxi
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@sourcegraph-bot
Copy link
Contributor

Codenotify: Notifying subscribers in OWNERS files for diff 1a2cb5e...dd11827.

Notify File(s)
@mrnugget dev/sg/internal/run/command.go
dev/sg/internal/run/run.go
dev/sg/internal/sgconf/config.go
dev/sg/sg_run.go
dev/sg/sg_start.go
@sourcegraph/dev-experience dev/sg/internal/run/command.go
dev/sg/internal/run/run.go
dev/sg/internal/sgconf/config.go
dev/sg/sg_run.go
dev/sg/sg_start.go

@jhchabran
Copy link
Member

jhchabran commented Jul 6, 2022

Right now, command env takes precedence over set env. I'm scared to change this, so instead I added envOverrides and prop-drilled it to override whatever env is set on the command.

Yeah, it broke stuff in the past, see #34895

I think we should get this sorted as you mentioned over Slack, before we slowly start turning it in a nightmare with exceptions mechanisms

@bobheadxi
Copy link
Member Author

bobheadxi commented Jul 6, 2022

I think we should get this sorted as you mentioned over Slack, before we slowly start turning it in a nightmare with exceptions mechanisms

I think this is not a bad way to go - define a precedence, and allow a few override mechanisms. Right now I think this might be the right band-aid

@bobheadxi
Copy link
Member Author

That said, I suppose there is a workaround without code changes - we set the env as part of the command script itself

@bobheadxi
Copy link
Member Author

Found a neater(?) hack: #38327

@bobheadxi bobheadxi closed this Jul 6, 2022
@bobheadxi bobheadxi deleted the 07-06-dev/sg_add_envOverrides_option_for_sets_to_override_command_env branch July 6, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants