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

DM-23711: Support shell variables in includeConfigs directive #234

Merged
merged 1 commit into from Mar 3, 2020

Conversation

timj
Copy link
Member

@timj timj commented Mar 3, 2020

No description provided.

@timj timj requested a review from TallJimbo March 3, 2020 17:59
Copy link
Member

@TallJimbo TallJimbo 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. I don't suppose you can easily do anything about this either way (except maybe document it), but do you know if expandvars will complain if referenced envvars don't exist, vs. just expanding them to "", the way shells typically (and unfortunately!) do?

@timj
Copy link
Member Author

timj commented Mar 3, 2020

It's documented to leave the variable in there untouched. Given that this is only used for reading configs you immediately get a big complaint about the file not being found.

@timj
Copy link
Member Author

timj commented Mar 3, 2020

I think complaining about that is a fairly obvious indicator of a problem since it will report the file name that includes the $

@timj
Copy link
Member Author

timj commented Mar 3, 2020

If you really want I can check for the presence of a $ and raise my own exception before the FileNotFoundError arrives.

@TallJimbo
Copy link
Member

No need for special checking. Leaving the var untouched should be enough of a diagnostic; I'm just glad it doesn't do what the shell does, as that has a much better chance of resolving to something incorrect that might exist.

@timj timj merged commit 2ce4b1b into master Mar 3, 2020
@timj timj deleted the tickets/DM-23711 branch March 3, 2020 23:36
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.

None yet

2 participants