-
-
Notifications
You must be signed in to change notification settings - Fork 259
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
Have bootstrap_pex_env pull in contents of pexrc files #747
Closed
rtkjliviero
wants to merge
8
commits into
pex-tool:main
from
rtkjliviero:include-pexrc-in-bootstrap-env
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
7451a62
Added optional arg to bootstrap_pex_env which will cause pexrc config…
rtkjliviero 00d34af
Adding list to list, instead of string to list
rtkjliviero 812c822
Style fix (line length)
rtkjliviero e03d719
Added option to pass in dict or json, for overriding specific vars in…
rtkjliviero 7d3691c
PR feedback; mostly Style cleanup
rtkjliviero a6ccc62
Reverted unnecessary style change
rtkjliviero ee4e27c
Added some unit tests for pex_bootstrapper and an empty .pex file as …
rtkjliviero 64bd609
Style fixes
rtkjliviero File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just accepting
**extra_vars
and pushing json back to the user that needs it. It's terrifying that we have a consumer of this API at all, and as a relunctant / conservative library maintainer I'd much rather keep API surface area minimal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sure what you mean here, would you mind clarifying?
Are you saying this should only allow properly formatted json here, and require the user to convert from a dict if necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsirois, if you could clarify I'm more than happy to update this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mean support the minimal api, which is allowing the user to pass environment variables via **extra_vars. If code that call this wants to support a json encoding of the env vars that code can decode the json to a dict to pass in as **extra_vars.
Concreteley, just:
Calling code in an application can do this if it want to unburdening Pex from supporting json in this API in perpetuity:
And, of course, where this is really important is in the public API function, so the **extra_vars style should propagate up to
bootstrap_pex_env
.