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

Opt-out for Git shallow clone in SCM feature #5677

Merged
merged 15 commits into from Aug 30, 2019

Conversation

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 27, 2019

Changelog: Feature: Add opt-out for Git shallow clone in SCM feature
Docs: conan-io/docs#1400

closes #5631


This PR introduces also:

  • validation of SCM data values
  • prevent bug using scm::verify_ssl and scm auto values
@jgsogo jgsogo changed the title refactor scm git checkout Opt-out for Git shallow clone in SCM feature Aug 27, 2019
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
@jgsogo jgsogo marked this pull request as ready for review Aug 27, 2019
@jgsogo jgsogo requested a review from lasote Aug 27, 2019
@memsharded memsharded added this to the 1.18.2 milestone Aug 28, 2019
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
conans/model/scm.py Outdated Show resolved Hide resolved
Loading
scm_data = SCMData(conanfile)
the_json = str(scm_data)
data2 = json.loads(the_json)
self.assertEqual(data, data2)
Copy link
Member Author

@jgsogo jgsogo Aug 29, 2019

Choose a reason for hiding this comment

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

This test was duplicated (see scm_serialization_test <-- it is the one modified above in this file)

Loading

conans/model/scm.py Outdated Show resolved Hide resolved
Loading
if key in data:
r = data.get(key)
if r is None: # None is always a valid value
return r
Copy link
Member Author

@jgsogo jgsogo Aug 29, 2019

Choose a reason for hiding this comment

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

None has to be a valid value right now. There are many recipes using functions like the following one to get SCM values:

def get_revision():
    try:
        return Git(...).get_commit()
    except:
        return None

Loading

self.type = _get_dict_value(data, "type", string_types)
self.url = _get_dict_value(data, "url", string_types)
self.revision = _get_dict_value(data, "revision", string_types + (int, ),
disallowed_type=bool) # bool is subclass of integer
Copy link
Member Author

@jgsogo jgsogo Aug 29, 2019

Choose a reason for hiding this comment

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

I leave int as a valid type for revision because SVN users might be using it this way (but I don't want a bool to be valid input).

Loading

return '"{}": {}'.format(key, value)
else:
value_str = str(value).replace('"', r'\"')
return '"{}": "{}"'.format(key, value_str)
Copy link
Contributor

@lasote lasote Aug 29, 2019

Choose a reason for hiding this comment

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

NoneType and int will be quoted too?

Loading

Copy link
Member Author

@jgsogo jgsogo Aug 29, 2019

Choose a reason for hiding this comment

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

  • Entries with None values are removed from the dictionary.
  • int will be quoted too, but IMO using int for revision (the only field that accepts it) should be discouraged too (it makes sense only for SVN)

Loading

Copy link
Contributor

@lasote lasote Aug 29, 2019

Choose a reason for hiding this comment

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

Then I think we should just transform to String anything it comes (even a function) and simplify the checks by checking only the Boolean fields (the SSL and the shallow).

Loading

Copy link
Contributor

@lasote lasote Aug 29, 2019

Choose a reason for hiding this comment

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

The checking code seems correct to me but not short nor simple, so, maybe it is too much.

Loading

Copy link
Member Author

@jgsogo jgsogo Aug 29, 2019

Choose a reason for hiding this comment

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

I don't think it will simplify a lot the code... it would be more or like this:

diff --git a/conans/model/scm.py b/conans/model/scm.py
index 172cf8a3a..c62fcc101 100644
--- a/conans/model/scm.py
+++ b/conans/model/scm.py
@@ -16,17 +16,17 @@ def get_scm_data(conanfile):
         return None
 
 
-def _get_dict_value(data, key, expected_type, default=None, disallowed_type=None):
+def _get_dict_value(data, key, expected_type=None, default=None):
     if key in data:
         r = data.get(key)
         if r is None:  # None is always a valid value
             return r
-        if not isinstance(r, expected_type) or (disallowed_type and isinstance(r, disallowed_type)):
+        if expected_type and not isinstance(r, expected_type):
             type_str = "' or '".join([it.__name__ for it in expected_type]) \
                 if isinstance(expected_type, tuple) else expected_type.__name__
             raise ConanException("SCM value for '{}' must be of type '{}'"
                                  " (found '{}')".format(key, type_str, type(r).__name__))
-        return r
+        return str(r) if expected_type is not bool else r
     return default
 
 
@@ -36,15 +36,14 @@ class SCMData(object):
 
     def __init__(self, conanfile):
         data = getattr(conanfile, "scm")
-        self.type = _get_dict_value(data, "type", string_types)
-        self.url = _get_dict_value(data, "url", string_types)
-        self.revision = _get_dict_value(data, "revision", string_types + (int, ),
-                                        disallowed_type=bool)  # bool is subclass of integer
+        self.type = _get_dict_value(data, "type")
+        self.url = _get_dict_value(data, "url")
+        self.revision = _get_dict_value(data, "revision")
         self.verify_ssl = _get_dict_value(data, "verify_ssl", bool, SCMData.VERIFY_SSL_DEFAULT)
-        self.username = _get_dict_value(data, "username", string_types)
-        self.password = _get_dict_value(data, "password", string_types)
-        self.subfolder = _get_dict_value(data, "subfolder", string_types)
-        self.submodule = _get_dict_value(data, "submodule", string_types)
+        self.username = _get_dict_value(data, "username")
+        self.password = _get_dict_value(data, "password")
+        self.subfolder = _get_dict_value(data, "subfolder")
+        self.submodule = _get_dict_value(data, "submodule")
         self.shallow = _get_dict_value(data, "shallow", bool, SCMData.SHALLOW_DEFAULT)
 
     @property

The drawback is that it will be converting to string (without errors) things that previously could be any type. Anyway, the only legit issue I can think of is the integer for an SVN revision, but I think it could be very marginal.

Shall I push these changes then? wdyt @memsharded?

Loading

@jgsogo jgsogo requested review from lasote and memsharded Aug 29, 2019
Copy link
Member

@memsharded memsharded left a comment

I agree with @lasote that the code could be simpler making it less generic, by assuming a default of strings, and just handling the corner cases of bools and ints explictly. At some points it becomes a bit difficult to read for the value.

However, it seems correct, lets move it forward now that we need to release 1.18.2, and @jgsogo will pay for 🍔 if something else breaks 😄

Loading

@lasote lasote merged commit 64c447b into conan-io:release/1.18.2 Aug 30, 2019
2 checks passed
Loading
@jgsogo jgsogo deleted the fix/optout-git-shallow branch Aug 30, 2019
AKhranovskiy pushed a commit to AKhranovskiy/conan that referenced this issue Sep 2, 2019
* refactor scm git checkout

* use shallow instead of shallow_clone as attrib name

* add tests for shallow

* reorder imporots

* remove local repo, it has to clone

* check if using shallow

* show output

* shallow does not work with commits

* check true positive

* better handling of SCMData fields

* many users may have values equal to 'None'

* six.string_types to str is different for PY2/3

* bool is subclass of int

* for PY2 bytes is a str

* forgot str substitution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants