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

Fix .env which broke as part of moving cmds to the project service #493

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

sourishkrout
Copy link
Member

@sourishkrout sourishkrout commented Feb 9, 2024

Please see #492 for details.

Before vs after:

❯ runme run hello1
SOMETHING1=[]
❯ ./runme run hello1
SOMETHING1=[this]

Fixes #492.

@sourishkrout sourishkrout linked an issue Feb 9, 2024 that may be closed by this pull request
@sourishkrout
Copy link
Member Author

sourishkrout commented Feb 9, 2024

@adambabik, please lemme know if this fix works for you. Please ignore the code coverage, it's low due to integration vs unit test (not in coverage report).

Instead of calling this in the Remote/Local runner's New, we could run it as part of applying the withProject option. Wdyt?

@@ -95,7 +99,6 @@ func (r *LocalRunner) newExecutable(task project.Task) (runner.Executable, error
Logger: r.logger,
}

// TODO(adamb): what about `r.envs`?
Copy link
Member Author

Choose a reason for hiding this comment

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

@adambabik sounded like you had a hunch here

@sourishkrout sourishkrout force-pushed the 492-env-loading-seems-broken-in-cli branch from 2f7cdcd to 7f9ad82 Compare February 9, 2024 16:59
@sourishkrout sourishkrout requested review from pastuxso and removed request for adambabik February 9, 2024 17:43
Copy link

sonarcloud bot commented Feb 9, 2024

@sourishkrout
Copy link
Member Author

@adambabik, please do a retro-review. Going through a review with @pastuxso right now so we can ship a fix quickly.

@sourishkrout sourishkrout merged commit c6b7703 into main Feb 9, 2024
6 checks passed
@sourishkrout sourishkrout deleted the 492-env-loading-seems-broken-in-cli branch February 9, 2024 17:53
@adambabik
Copy link
Collaborator

@sourishkrout I think it's ok. The way the environment variables are handled in internal/runner/client is a bit unclear, but in the end all env go to a session which is passed to the runner via ExecutableConfig and from there the env are eventually taken.

I will send a follow up PR with clean up.

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.

.env loading seems broken in CLI
3 participants