Skip to content

add remotes in constructor #359

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 2, 2019
Merged

add remotes in constructor #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpt/packager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ def __init__(self, username=None, channel=None, runner=None,

self.upload_only_recipe = upload_only_recipe or get_bool_from_env("CONAN_UPLOAD_ONLY_RECIPE")

self.remotes_manager.add_remotes_to_conan()
self.uploader = Uploader(self.conan_api, self.remotes_manager, self.auth_manager,
self.printer, self.upload_retry)

Expand Down Expand Up @@ -457,7 +458,6 @@ def run(self, base_profile_name=None):
self.printer.print_message("Skipped builds due [skip ci] commit message")
return 99
if not self.skip_check_credentials and self._upload_enabled():
self.remotes_manager.add_remotes_to_conan()
self.auth_manager.login(self.remotes_manager.upload_remote_name)
if self.conan_pip_package and not self.use_docker:
with self.printer.foldable_output("pip_update"):
Expand Down
1 change: 0 additions & 1 deletion cpt/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ def __init__(self, profile_abs_path, reference, conan_api, uploader,
self._exclude_vcvars_precommand = exclude_vcvars_precommand
self._build_policy = build_policy
self._runner = PrintRunner(runner or os.system, self.printer)
self._uploader.remote_manager.add_remotes_to_conan()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to break something when running docker. Have you tried?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I haven't. Also, currently I am basically in the middle of nowhere, so I can't test it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it will. I see these ways to fix this:

  1. Add remotes both in packager's and in runner's constructors. This will print a message about remote already existing for each remote if not running jobs in Docker.
  2. Same as 1, but add a flag to RemotesManager, which is set and checked in add_remotes_to_conan.
  3. Add remotes in RemotesManager's constructor.
  4. Add remotes in packager's constructor and in cpt.run_in_docker:run.

What is the most desired approach? Personally, I like approach 2 the most.

self._test_folder = test_folder
self._config_url = config_url
self._upload_only_recipe = upload_only_recipe
Expand Down
10 changes: 2 additions & 8 deletions cpt/test/unit/packager_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,6 @@ def test_remotes(self):
reference="lib/1.0@lasote/mychannel",
ci_manager=self.ci_manager)

builder.add({}, {}, {}, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests if remotes are added. Currently, remotes are added when the jobs are being run. With this change remotes are added before that, so there's no point in adding any jobs.

builder.run_builds()
self.assertEquals(self.conan_api.calls[1].args[1], "url1")
self.assertEquals(self.conan_api.calls[1].kwargs["insert"], -1)
self.assertEquals(self.conan_api.calls[3].args[1], "url2")
Expand All @@ -485,8 +483,6 @@ def test_remotes(self):
reference="lib/1.0@lasote/mychannel",
ci_manager=self.ci_manager)

builder.add({}, {}, {}, {})
builder.run_builds()
self.assertEquals(self.conan_api.calls[1].args[1], "myurl1")
self.assertEquals(self.conan_api.calls[1].kwargs["insert"], -1)

Expand All @@ -502,8 +498,6 @@ def test_remotes(self):
reference="lib/1.0@lasote/mychannel",
ci_manager=self.ci_manager)

builder.add({}, {}, {}, {})
builder.run_builds()
self.assertEquals(self.conan_api.calls[1].args[0], "my_cool_name1")
self.assertEquals(self.conan_api.calls[1].args[1], "u1")
self.assertEquals(self.conan_api.calls[1].kwargs["insert"], -1)
Expand Down Expand Up @@ -710,7 +704,7 @@ def test_check_credentials(self):

# When activated, check credentials before to create the profiles
self.assertEqual(self.conan_api.calls[2].name, 'authenticate')
self.assertEqual(self.conan_api.calls[5].name, 'create_profile')
self.assertEqual(self.conan_api.calls[3].name, 'create_profile')

self.conan_api = MockConanAPI()
# If we skip the credentials check, the login will be performed just before the upload
Expand All @@ -725,7 +719,7 @@ def test_check_credentials(self):
ci_manager=self.ci_manager)
builder.add_common_builds()
builder.run()
self.assertNotEqual(self.conan_api.calls[0].name, 'authenticate')
self.assertNotEqual(self.conan_api.calls[2].name, 'authenticate')

# No upload, no authenticate
self.conan_api = MockConanAPI()
Expand Down