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

[Feature] Allow Environment Override #1842

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

avi-jois
Copy link

@avi-jois avi-jois commented Apr 6, 2023

Allow the pip environment used for evaluating environment markers to be overriden, so requirements can be compiled for an environment different than the user's current environment.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

Copy link

@aignas aignas left a comment

Choose a reason for hiding this comment

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

I am not a maintainer, but would love to see this change to be merged.

)

assert out.exit_code == 0, out
assert expected_output == out.stdout
Copy link

Choose a reason for hiding this comment

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

I would love to see an extra test, where we check that we can override all of the env dict values to ensure that we support passing multiple values to the --override-environment

Copy link
Author

Choose a reason for hiding this comment

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

I added a test with multiple environment markers passed, as well as a test that uses all valid markers one at a time.

@click.option(
"--override-environment",
multiple=True,
type=(str, str),
Copy link

Choose a reason for hiding this comment

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

can we add validation on the values that people can override? I would expect this to fail if I pass --override-environment foo bar to the CLI. If this can be shown in the help to tell the user what environment markers can be overridden, it would improve the experience as well.

Copy link
Author

Choose a reason for hiding this comment

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

Added argument validation

specify the environment when gathering dependencies, allowing for cross-environment
fetching. However, a different ``requirements.txt`` must still be generated per
environment. It is recommended to override all keys in `PEP 508 environment markers`_
when targetting a different environment so the environment is fully defined.
Copy link

Choose a reason for hiding this comment

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

It would be great to have a short example in the README.

Copy link
Author

Choose a reason for hiding this comment

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

Added an example for a 'typical' Linux machine

Allow the pip environment used for evaluating environment markers to be
overriden, so requirements can be compiled for an environment different
than the user's current environment.
@avi-jois
Copy link
Author

@aignas Thanks for the comments! I made some updates based on your feedback, let me know if there are any remaining concerns you have.

@atugushev I saw you are one of the leads on this project, do you think this feature makes sense?

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.

2 participants