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 username & password in SCM for git clone with ssh #8016

Merged
merged 16 commits into from Nov 25, 2020

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Nov 5, 2020

Changelog: Fix: Set username or password individually in git SCM with ssh.
Docs: omit

  • Refer to the issue that supports this Pull Request: closes [bug] SCM username value is ignored #8011
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@danimtb danimtb self-assigned this Nov 5, 2020
@danimtb danimtb added this to the 1.32 milestone Nov 11, 2020
@danimtb danimtb assigned jgsogo and unassigned danimtb Nov 11, 2020
@danimtb danimtb marked this pull request as ready for review November 11, 2020 15:45
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

There are a couple of tests

@jgsogo jgsogo assigned czoido and unassigned jgsogo Nov 11, 2020

def test_ssh_password(self):
scm = SCMBase(password="pass")
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused a bit about this test. So adding the password, but not the user, will not add the password, but will not error either?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the behavior before but I am not sure how the user could set only the password. Maybe providing the user already in the URL?

Copy link
Member

Choose a reason for hiding this comment

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

Well, that is the point, the previous behavior was requiring both user+password. Now we changed it, and adding only the user is a new valid behavior, with a new test checking it.

I'd say that we need to define a new behavior for only the password: either an error for this case, or check that there is a valid scenario, like the user is already in the URL, but that should probably be a new test too.

@danimtb
Copy link
Member Author

danimtb commented Nov 12, 2020

I have added some warning messages for options that ignore parameters

@danimtb danimtb requested a review from czoido November 16, 2020 16:34
conans/client/tools/scm.py Outdated Show resolved Hide resolved
@danimtb

This comment has been minimized.

@danimtb danimtb requested a review from czoido November 20, 2020 12:32
@danimtb danimtb assigned jgsogo and unassigned czoido Nov 20, 2020
@danimtb danimtb requested a review from jgsogo November 20, 2020 13:04
conans/client/tools/scm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

My spider-sense says this is going to fail somewhere and somehow. I find too many if/else here. I'd suggest a dispatcher, something like:

def handle_scp_pattern(user, domain, url):
    ... logic related to user, password and warnings.
    return "{usr}@{domain}:{url}".format(user=user, domain=domain, url=url)

def handle_url_pattern(protocol, url, user=None, password=Non):
    ... logic related to user, password and warnings
    auth = "{user}:{password}@" if xxxx else "{user}@" if yyyyy else ""
    return "{protocol}://{auth}{url}".format(protocol=protocol, auth=auth, url=url)

# Maybe other actions if the logic inside of them starts to be cumbersome

# Now a map/list we will iterate and use the matching dispatcher
url_patterns = {
   'scp': [
      re.compile("^(?P<user>[a-zA-Z0-9_]+)@(?P<domain>[a-zA-Z0-9._-]+):(?P<url>.*)$"),
      handle_scp_pattern
   ]
   'url_user_pass': [
      re.compile("^(?P<protocol>file|http|git)s?:\/\/(?P<user>\S+):(?P<password>\S+)@(?P<url>.*)$"),
      handle_url_pattern
   ]
   'url_user': [
      re.compile("^(?P<protocol>file|http|git)s?:\/\/(?P<user>\S+)@(?P<url>.*)$"),
      handle_url_pattern
   ]
}

for key, (pattern, action) in url_patterns:
    m = pattern.match(url)
    if m:
        return action(**m.groupdict())

self.output.warn("URL not recognized....")
return url

wdyt? This is just a suggestion. Let me know and I'll review carefully whatever you choose

@danimtb
Copy link
Member Author

danimtb commented Nov 23, 2020

@jgsogo I have followed your suggestion and I think the code is cleaner. Please check again as I have also added more tests. Thank you!

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Some suggestions. Now it's much easier to follow all the conditions 🎉

conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved
danimtb and others added 5 commits November 24, 2020 18:19
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
@jgsogo jgsogo assigned czoido and unassigned jgsogo Nov 24, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This is much better structured and understandable, and likely to be more robust. Well done.

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.

[bug] SCM username value is ignored
4 participants