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

config install arguments for git clone and save in config #4083

Merged
merged 11 commits into from Dec 18, 2018

Conversation

Projects
None yet
4 participants
@danimtb
Copy link
Member

commented Dec 7, 2018

Changelog: Feature: Allow arguments in git clone for conan config install
Docs: conan-io/docs#975

  • Refer to the issue that supports this Pull Request: closes #3656

  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
    This stored the general.config_install config in a new format git:[url.com/conan.git, -b 0.0.1] that allows reusing the full commands for clone with just conan config install (without repeating arguments).

    This could be useful too for Conan hooks, as people might have them as submodules in a git repo and you could clone them with conan config install url.com/conan.git --args "--recusrive"

  • 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.

@ghost ghost assigned danimtb Dec 7, 2018

@ghost ghost added the stage: review label Dec 7, 2018

@danimtb danimtb changed the title Feature/3656 config install arguments for git clone and save in config Dec 7, 2018

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

I have beeing doing some test with the --recursive flag for submodules and it is very messy as you have to copy the whole .git folder with tons of files. However, I still think this is a useful feature for people that want to use different tags/branches in git

@danimtb danimtb assigned jgsogo and unassigned danimtb Dec 10, 2018

@jgsogo
Copy link
Member

left a comment

Unittesting is important ;D

Show resolved Hide resolved conans/client/conf/config_installer.py
Show resolved Hide resolved conans/client/conf/config_installer.py Outdated

@ghost ghost assigned danimtb Dec 10, 2018

@danimtb danimtb removed their assignment Dec 11, 2018

@jgsogo

jgsogo approved these changes Dec 11, 2018

@jgsogo jgsogo added this to the 1.11 milestone Dec 11, 2018

@danimtb danimtb referenced this pull request Dec 11, 2018

Merged

Management of hooks folders and naming #4106

5 of 5 tasks complete

@ghost ghost assigned danimtb Dec 11, 2018

danimtb added some commits Dec 11, 2018

@danimtb danimtb requested a review from lasote Dec 13, 2018

@lasote

lasote approved these changes Dec 17, 2018

Copy link
Contributor

left a comment

Solve conflicts and merge

def config_install(self, item, verify_ssl, config_type=None, args=None):
# _make_abs_path, but could be not a path at all
if item is not None and os.path.exists(item) and not os.path.isabs(item):
item = os.path.abspath(item)

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 18, 2018

Contributor

This is removed, but seems it was in place for something. Maybe it is not covered by tests, but please check why it was removed.

This comment has been minimized.

Copy link
@danimtb

danimtb Dec 18, 2018

Author Member

This has been moved here: https://github.com/conan-io/conan/pull/4083/files#diff-32564897949777992206da1e43543b08R127 because the item is no longer a path or url, but a structure with type of config, address (path or url) and arguments. I am sure it is covered by the tests below

Show resolved Hide resolved conans/client/tools/scm.py Outdated
else:
raise ConanException("Unable to process config install: %s" % path_or_url)
else:
config_type, rest = item.split(":", 1)

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 18, 2018

Contributor

I think this will break for existing data in conan.conf that it will only contain 1 item. Please check and add tests if necessary

This comment has been minimized.

Copy link
@danimtb

danimtb Dec 18, 2018

Author Member

This PR takes into account the possibility of having still one item in the conan.conf. Have alook again at https://github.com/conan-io/conan/pull/4083/files#diff-32564897949777992206da1e43543b08R168

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 18, 2018

Contributor

Ok, you are right, I was misreading the code.

@danimtb danimtb removed their assignment Dec 18, 2018

else:
raise ConanException("Unable to process config install: %s" % path_or_url)
else:
config_type, rest = item.split(":", 1)

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 18, 2018

Contributor

Ok, you are right, I was misreading the code.

@memsharded memsharded merged commit 52c3346 into conan-io:develop Dec 18, 2018

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 Dec 18, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Merge pull request conan-io#4083 from danimtb/feature/3656
config install arguments for git clone and save in config
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.