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

Rollout flags (--use-feature, --use-deprecated) #8530

Merged
merged 5 commits into from
Jul 4, 2020

Conversation

pradyunsg
Copy link
Member

This implements the feature rollout approach we have concensus on (from #8371) and handles everything except the warnings for #8513.

AFAICT, there's gonna be 2 more follow up PRs to this:

Since we'd have a dedicated documentation PR, I suggest we skip the changelog entry for this PR.

@pradyunsg pradyunsg added type: enhancement Improvements to functionality skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Jul 3, 2020
Co-authored-by: Stéphane Bidoul <[email protected]>
@pradyunsg pradyunsg changed the title Rollout flags (--use-feature, --deprecated-feature) Rollout flags (--use-feature, --use-deprecated) Jul 3, 2020
@pradyunsg
Copy link
Member Author

@ei8fdb @nlhkabu With this PR, if the user passes --unstable-feature=resolver they get the following error:

Screenshot 2020-07-03 at 6 37 47 PM

@pradyunsg
Copy link
Member Author

I'm gonna go ahead and merge this, since there's multiple green ticks, and the specific wording can be iterated upon in a follow up PR. :)

@pradyunsg pradyunsg merged commit 8db4fc8 into pypa:master Jul 4, 2020
@pradyunsg pradyunsg deleted the rollout-flags branch July 4, 2020 20:20
@McSinyx
Copy link
Contributor

McSinyx commented Jul 5, 2020

Screenshot 2020-07-03 at 6 37 47 PM

This also shows up if the option was set using environmental variable (PIP_UNSTABLE_FEATURE=resolver). Is there a way to differentiate the way it is passed, since for me for instance the variable is loaded upon login and I ended up having to relogin to unset it?

@pradyunsg
Copy link
Member Author

Is there a way to differentiate the way it is passed

If it's coming from anything other than the command line, pip config list should show you whether it's declared in a file or as an environment variable.

@McSinyx
Copy link
Contributor

McSinyx commented Jul 5, 2020

Thanks, let me clarify my intention: my concern was about people setting it e.g. in .profile which is loaded upon multi-user target login—they wouldn't be able to disable it easily (I had to at least kill GUI to get back to tty to unset the variable). I wonder if we want to have a warning instead, at least if the option is set using an environment variable when we roll this out to avoid disruptive experience I had earlier today.

@pfmoore
Copy link
Member

pfmoore commented Jul 5, 2020

I'd argue that people shouldn't be setting an "unstable feature" flag globally, and if they do, we really don't have much obligation to help them deal with the consequences 🙂

Having said that, if it's easy for the code that generates the warning to tell if the option came from the command line or not, I see no harm adding that detail to the warning.

@pradyunsg
Copy link
Member Author

This is a flag that:

  • explicitly contains "unstable" in the name
  • is not exposed in any of the CLI help documentation
  • was explicitly communicated as for testing the in-development version of the resolver.

I don't think people are setting this in multi-user profiles, and that is not something common for testing unstable functionality. I don't think we need to handle this flag differently due to this edge case - these users are using an unstable feature, with additional complexity on top from other tooling. They are opting in for dealing with these complexities (either themselves or by someone else in control of their systems).

I'll note that this PR is the result of a long discussion on the rollout (see #8371), and is part of something that has been discussed at some length already. If you'd like to discuss this further, I suggest filing a new issue instead of making additional comments on this PR.

@McSinyx
Copy link
Contributor

McSinyx commented Jul 6, 2020

I'd argue that people shouldn't be setting an "unstable feature" flag globally, and if they do, we really don't have much obligation to help them deal with the consequences 🙂

I don't think people are setting this in multi-user profiles, and that is not something common for testing unstable functionality. I don't think we need to handle this flag differently due to this edge case - these users are using an unstable feature, with additional complexity on top from other tooling. They are opting in for dealing with these complexities (either themselves or by someone else in control of their systems).

I guess this is the part of the discussion that I missed 😄 Since I've found a way to counter this and it's unlikely that anyone else would do what I did, I think I'm satisfied with the decisions and the explanations.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes) type: enhancement Improvements to functionality type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants