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 credentials necessary for install #4872

Merged
merged 5 commits into from Apr 1, 2019

Conversation

Projects
None yet
2 participants
@memsharded
Copy link
Contributor

commented Mar 29, 2019

Changelog: Bugfix: Client does not require credentials for anonymous downloads from remotes.
Docs: omit

Close #4871

@tags: slow

cc/ @fschoenm

@memsharded memsharded added this to the 1.14.1 milestone Mar 29, 2019

@ghost ghost assigned memsharded Mar 29, 2019

@ghost ghost added the stage: review label Mar 29, 2019

@memsharded memsharded assigned lasote and unassigned memsharded Mar 29, 2019

@ghost ghost assigned memsharded Mar 29, 2019

memsharded added some commits Mar 29, 2019

@lasote
Copy link
Contributor

left a comment

The fix is ok, but I don't know if it is safer to move that check to the uploader to avoid future confusions or "incorrect" usage of the get_recipe_snapshot WDYT?

@@ -162,15 +162,15 @@ def upload_recipe(self, ref, files_to_upload, deleted, retry, retry_wait):
self._remove_conanfile_files(ref, deleted)

def get_recipe_snapshot(self, ref):
# this method is used only for UPLOADING, then it requires the credentials

This comment has been minimized.

Copy link
@lasote

lasote Mar 31, 2019

Contributor

Probably it would be better to move it to the uploaded just before calling this method:


    def _recipe_files_to_upload(self, ref, policy, the_files, remote, remote_manifest):
        # Get the remote snapshot
        # ===> HERE call the check_credentials?
        remote_snapshot = self._remote_manager.get_recipe_snapshot(ref, remote)

This comment has been minimized.

Copy link
@lasote

lasote Mar 31, 2019

Contributor

It would require to make available that method in the auth_manager and remote_manager

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 31, 2019

Author Contributor

Done

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 31, 2019

Author Contributor

it just reads a bit weird:

class ConanApiAuthManager(object):

    @input_credentials_if_unauthorized
    def check_credentials(self):
        self._rest_client.check_credentials()

We might need to revise this pattern sometime, but not urgent

This comment has been minimized.

Copy link
@lasote

lasote Apr 1, 2019

Contributor

Absolutely agree.

@lasote lasote merged commit e5ff8f6 into conan-io:release/1.14.1 Apr 1, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Apr 1, 2019

@memsharded memsharded deleted the memsharded:fix/credentials_install branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.