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

Add the --trusted option to upload and register commands #463

Closed
wants to merge 8 commits into from

Conversation

glenfant
Copy link

@glenfant glenfant commented May 21, 2019

This is explained in issue #387 and helps people who prefer to ignore CA cert issues and skip server certificate verifications.
Any comment is welcome.

Fixes #387.

.gitignore Outdated Show resolved Hide resolved
twine/repository.py Outdated Show resolved Hide resolved
twine/settings.py Outdated Show resolved Hide resolved
glenfant pushed a commit to glenfant/twine that referenced this pull request Jun 5, 2019
@glenfant
Copy link
Author

glenfant commented Jun 5, 2019

Hi, sorry to fix all this so late, but these are done.

@glenfant
Copy link
Author

glenfant commented Jun 7, 2019

I don't understand this failure. @sigmavirus24 Can you please explain the required fix ?

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

@glenfant From the link below, it looks like the failure is due to line length. I've made some suggestions to correct it.

https://travis-ci.org/pypa/twine/jobs/541750124#L235-L236

I got here by clicking the "Details" link in the "All checks have failed box" at the bottom of the PR, and then clicking on the "Linting code smells" failure.

twine/settings.py Outdated Show resolved Hide resolved
twine/commands/upload.py Outdated Show resolved Hide resolved
@di
Copy link
Sponsor Member

di commented Jul 23, 2019

FYI @glenfant you have merge conflicts here.

glenfant and others added 5 commits July 31, 2019 12:01
Added the "--tusted" option (WIP)

Added '--trusted' option on upload and register
Line length fixed

Co-Authored-By: Brian Rutledge <bhrutledge@gmail.com>
glenfant pushed a commit to glenfant/twine that referenced this pull request Jul 31, 2019
@glenfant
Copy link
Author

Hi @bhrutledge all your comments are taken into account. Ready to merge !
Cheers.

@bhrutledge
Copy link
Contributor

@glenfant Thanks for your continued work on this. However, this PR is now showing a bunch of changes that have already been merged. I don't know how that happened, or how to fix it, but I'm wary of merging this as-is.

If it were me, I'd probably close this PR, create a new branch off a fresh fetch of upstream/master, cherry-pick your changes, and submit a new PR. But there may be a better resolution.

@sigmavirus24
Copy link
Member

git rebase would be the better solution. @glenfant let me know if you'd like instructions.

@bhrutledge bhrutledge self-requested a review September 7, 2019 12:04
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Flagging this as needing changes before merging, namely git rebase.

@bhrutledge
Copy link
Contributor

@glenfant Are you able to do the rebase? If not, a Twine maintainer can probably help and/or do it.

@bhrutledge bhrutledge requested a review from di October 5, 2019 19:21
@bhrutledge
Copy link
Contributor

@di has some concerns about this feature: #387 (comment).

@jaraco
Copy link
Member

jaraco commented Oct 30, 2019

this PR is now showing a bunch of changes that have already been merged. I don't know how that happened

It looks like it may have been me. I didn't intentionally merge, but it looks like in add0aea, I merged in unrelated commits. The solution would be to cherrypick the correct commits without that merge commit. I can do that.

@jaraco
Copy link
Member

jaraco commented Oct 30, 2019

On further consideration, and looking at the actual commits in glenfant:master, I don't see that commit in his commit history... but what I do see is f54ea50, which merges two heads that seem to be doing the same thing. It looks like a rebase was attempted, then merged with itself... and the way github is detecting the ancestry, it's comparing against the wrong basis.

image

Now I think the fix is to cherry-pick 8ee8bb2 onto 2da05c4 and then force-push that.

@jaraco
Copy link
Member

jaraco commented Oct 30, 2019

I've fixed the repo history and resolved the conflicts with recent merges. Still, there's a pending concern about this change, so I'll wait for that to be resolved.

@jaraco
Copy link
Member

jaraco commented Nov 21, 2019

I think I agree with the sentiments of di. I'd like for this feature to be an ugly hack and not a simple option. What if instead you had this script:

import twine.__main__


def disable_server_certificate_validation():
    "Allow twine to just trust the hosts"
    twine.repository.Repository.set_certificate_authority = lambda *args, **kwargs: None


def main():
    disable_server_certificate_validation()
    twine.__main__.main()

__name__ == '__main__' and main()

Call it twine-trusted and invoke that. We could consider supplying disable_server_certificate_validation with twine (so as to always match the API), but it still requires an ugly hack to invoke, providing the sufficient level of discouragement to make any user seriously consider using it.

@okainov
Copy link

okainov commented Dec 5, 2019

Would be great to have it merged, thanks!

@jaraco
Copy link
Member

jaraco commented Dec 15, 2019

I think the consensus on this issue is that the bypassing of the trusted mode can be readily handled by an intentionally-ugly workaround for environments that require it.

@jaraco jaraco closed this Dec 15, 2019
@agates4
Copy link

agates4 commented Oct 13, 2021

any change of opinion?

this would be a fantastic feature to have.

@sigmavirus24
Copy link
Member

No

@pypa pypa locked as resolved and limited conversation to collaborators Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upload to untrusted hosts with --trusted option
7 participants