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

Job, possibility to update env vars #180

Merged
merged 8 commits into from
Feb 13, 2024
Merged

Conversation

albertz
Copy link
Member

@albertz albertz commented Feb 10, 2024

As it was discussed in #178.

Fix #178

Copy link
Contributor

@critias critias left a comment

Choose a reason for hiding this comment

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

See my comment on #178

I think putenv should be named set_env and I would avoid creating and compiling an extra file here and just update directly from _sis_environ_updates.

@albertz albertz requested a review from critias February 11, 2024 21:01
@critias
Copy link
Contributor

critias commented Feb 11, 2024

It bothers me to introduce a new structure which changes the environment. How about always creating the EnvironmentModifier, but only clean the environment if gs.CLEANUP_ENVIRONMENT is True and set_env would just pass the envs to EnvironmentModifier.set.

That way you should be able to accomplish the same thing without updating the environment in two different places.

@albertz
Copy link
Member Author

albertz commented Feb 11, 2024

I really don't want the string.Template.substitute logic though, so in any case, I need another flag to enable or disable that, maybe per key, or globally. I think per key might be better, because I don't want to break any existing setups, or introduce the situation that you cannot mix it, thus have it incompatible with some setups where you both need this for some env vars, and where you don't want this for other env vars.

@critias
Copy link
Contributor

critias commented Feb 11, 2024

Let's move the discussion to #178 to avoid two parallel discussions about the same thing.

@albertz albertz changed the title Job startup hook, to update env vars Job, possibility to update env vars Feb 11, 2024
sisyphus/job.py Outdated Show resolved Hide resolved
sisyphus/job.py Outdated Show resolved Hide resolved
@albertz albertz requested a review from critias February 12, 2024 21:43
@albertz albertz merged commit af0edef into master Feb 13, 2024
3 checks passed
@albertz albertz deleted the albert-custom-job-env-178 branch February 13, 2024 10:12
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.

Possibility to set specific env vars for job
3 participants