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

Export user variables before their definition #2485

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

TomekTrzeciak
Copy link
Contributor

It would be useful to have user environment variables exported as soon as they are defined, so they could be used in further definitions like so:

FOO='foo'
BAR=$(script_using_FOO)

instead of:

FOO='foo'
BAR=$(export FOO; script_using_FOO)

This pull request moves the export of variables before their definition, which makes them exported as soon as they are defined.

@TomekTrzeciak
Copy link
Contributor Author

TomekTrzeciak commented Nov 16, 2017

There are currently two tests failing - tests/env-filter/00-filter.t and tests/jobscript/00-torture.t . Both of them rely on the current structure of the jobscript, so this is expected, but before changing anything in tests I'll wait for the feedback on this pull request.

@matthewrmshin matthewrmshin added this to the next release milestone Nov 16, 2017
@hjoliver
Copy link
Member

@TomekTrzeciak - at first glance, this seems a good idea, thanks. There seems to be a lot of spurious commits on your branch; perhaps you need to rebase it onto master?

@TomekTrzeciak
Copy link
Contributor Author

Sorry, I've botched my local branch and then pushed that to github. The spurious commits should be gone now and the tests should be updated as well.

@hjoliver hjoliver self-requested a review November 17, 2017 04:16
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks good; tests as working.

@matthewrmshin
Copy link
Contributor

(Change is harmless and is an improvement to the intention of the current functionality. My normal comment (e.g. metomi/rose#1221) applies when it comes to the motivation of the use case, however.)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Existing tests provide adequate testing for this change.

@oliver-sanders oliver-sanders merged commit 2847b3d into cylc:master Nov 17, 2017
@TomekTrzeciak TomekTrzeciak deleted the job_env_export branch November 17, 2017 12:53
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.

4 participants