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

Fix flake8 complaints regarding except w/o arg #30

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

webknjaz
Copy link
Member

Ref #28, @pfmoore asked to submit it separately.

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2018

My only concern here is whether except Exception does what we want on Python 2.6. I expect that it does, but I don't have Python 2.6 myself to check, and docs.python.org doesn't go back as far as 2.6. I think the codebase needs to retain 2.6 compatibility, because we provide a Python 2.6 version of get-pip.py.

Assuming there's no issue on 2.6, I'm happy for this to be merged.

@webknjaz
Copy link
Member Author

@webknjaz
Copy link
Member Author

it's just missing from drop-down, but you can still access docs for 2.6

@webknjaz
Copy link
Member Author

Also, during work on #23 we tested it in CI and it didn't cause any problems.

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2018

it's just missing from drop-down, but you can still access docs for 2.6

Thanks for the pointer. Looks like the change I was thinking of was in 2.5, so we're fine here.

@pfmoore pfmoore merged commit 80780a6 into pypa:master Apr 24, 2018
@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2018

This was a mistake. Modifying get-pip.py directly is not correct - the changes will be lost on a rebuild as the actual code that should have been fixed is in the template directory.

Rather than simply copying these changes into the templates, I'd prefer to hold off on any further "quick fixes" here in favour of better documenting the workflow needed in this repository. Changes here directly affect production users of get-pip - there's no concept of a "release" of get-pip.py, so far more caution in what we merge into master is indicated.

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2018

Um, re-reviewing this, I'd forgotten that the templates were also updated (I did see that on my initial review). Nevertheless, the point remains that direct changes to get-pip.py are not something we should be doing lightly, and should almost always be restricted to when a pip release is being made (in my view) - typically via the generate.installers task.

@webknjaz
Copy link
Member Author

@pfmoore you are completely right about this. I missed that as well.

Honestly I was going to start conversation about:

  1. Adding Travis CI into deployment process (it's very easy to make it triggerable via tag push) or at least testing code generation there. In my opinion it eliminates human error factor in a best way possible.
  2. Not storing generated files in Git at all. Once there's automation + documented generation process, we just don't need to store artifacts, which we can easily get from sources. It's quite common practice, why not apply it here as well?

@pfmoore
Copy link
Member

pfmoore commented Apr 24, 2018

@webknjaz That would be worth a conversation, certainly. Taking your points in the opposite order:

  1. We can't (easily) stop storing the generated files in git, as that's where https://bootstrap.pypa.io/ gets them from. I don't know what the mechanism for that is, or who controls it - likely @dstufft - but as things stand, dropping the generated scripts would break deployment.
  2. As a consequence of that, a simple push to master currently deploys the scripts (see https://pip.pypa.io/en/latest/development/#release-process step 8). So CI checks happen basically way too late, and we don't tag on this repo, as there's simply no point. As things stand, CI wouldn't eliminate human error - at best it would show after the fact that you screwed up.

Basically, the way this repository works is pretty non-standard, and changes that affect workflow need to be carefully thought out. I'm new to all this myself, until I did the pip 10 release, I wasn't really aware of how any of this worked.

@hugovk
Copy link
Contributor

hugovk commented Apr 24, 2018

A huge benefit of a CI is to check things at PR stage, so before things even get to master (and delploy).

Having said that, it may not apply here due to things being non-standard, but if it might catch something, that's a plus.

@webknjaz
Copy link
Member Author

@pfmoore I completely understand and agree that things should not be swapped immediately.
First there should be better understanding of how it works now and to what extent it's acceptable to change things.
Maybe it already relies on GitHub Deployments API, which already enables it to depend on CI status. I don't know that, but will be happy to help with improvements.
Also, I think I'd switch to generating artifacts as part of CI/CD and trigger builds using git tags, which could as well correspond to currently bundled pip version, for example.

@dstufft
Copy link
Member

dstufft commented Apr 25, 2018

We can't (easily) stop storing the generated files in git, as that's where https://bootstrap.pypa.io/ gets them from. I don't know what the mechanism for that is, or who controls it - likely @dstufft - but as things stand, dropping the generated scripts would break deployment.

It's controlled in https://github.com/python/psf-salt/blob/master/salt/pypa/bootstrap/init.sls. It can be modified, though some people may be pulling it directly from GitHub I dunno.

@webknjaz
Copy link
Member Author

@dstufft it might be worth dropping support for downloads from gh for the sake of securing deploy flow, what do you think?

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

4 participants