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

Implement add_repository for SystemPackageTool (only AptTool) (close #3385) #3445

Merged
merged 8 commits into from Sep 17, 2018

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Aug 30, 2018

  • close #3385
  • 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. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs
  • Add test
  • Someone must confirm it actually works in production for AptTool (test is checking the command, but there might be a typo)

Following last comments by @lasote here this pr implements the add_repository functionality for SystemPackageTool (only available for AptTool).

Changelog: Feature: Improved AptTool at SystemPackageTool adding a function add_repository to add new apt repositories.

@ghost ghost assigned jgsogo Aug 30, 2018

@ghost ghost added the stage: review label Aug 30, 2018

@@ -54,6 +55,21 @@ def _create_tool(os_info):
else:
return NullTool()

def add_repositories(self, repositories, repo_keys=None, update=True):

This comment has been minimized.

Copy link
@lasote

lasote Aug 31, 2018

Contributor

Personally, I would prefer the user calling N times to add_repository and avoid the complex parameters. I think typically they will add 1 or 2 max.

@lasote lasote added this to the 1.8 milestone Sep 3, 2018

@@ -140,6 +154,9 @@ def installed(self, package_name):


class YumTool(object):
def add_repository(self, repository, repo_key=None):
raise ConanException("YumTool::add_repository not implemented (see #3385)")

This comment has been minimized.

Copy link
@lasote

lasote Sep 3, 2018

Contributor

Not sure about the error message with the #3385. WDYT @memsharded? Too much coupled to GH in my opinion.

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 3, 2018

Contributor

Yes, I wouldn't use it here. Because #3385 will be closed as implemented, too, which might be confusing. Too much coupled.

spt.add_repository(repository=repository, repo_key=None, update=False)
self.assertTrue(len(runner.commands) == 0)

with tools.environment_append({"CONAN_SYSREQUIRES_SUDO": "False"}):

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 3, 2018

Contributor

Too much repetition, couldn't it be a parameterized test?

spt = SystemPackageTool(runner=runner, os_info=os_info)

spt.add_repository(repository=repository, repo_key=gpg_key, update=update)
self.assertTrue(len(runner.commands) == 0)

This comment has been minimized.

Copy link
@lasote

lasote Sep 5, 2018

Contributor

Tip (not important): self.assertEquals(len(runner.commands), 0) would be more correct.

@lasote

lasote approved these changes Sep 5, 2018

@memsharded memsharded assigned memsharded and unassigned jgsogo Sep 17, 2018

@memsharded memsharded merged commit bb404b3 into conan-io:develop Sep 17, 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 Sep 17, 2018

@jgsogo jgsogo deleted the jgsogo:issue/3385 branch Sep 19, 2018

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

Merge pull request conan-io#3445 from jgsogo/issue/3385
Implement add_repository for SystemPackageTool (only AptTool) (close conan-io#3385)
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.