Adding mirror true check for rpm/file plugin #42

ragabala · 2019-03-20T17:23:41Z

This commit will test cross plugin scenario. If the user syncs multiple
plugin contents into the same repo with the option mirror=True, the
existing content should be overwritten.

Closes #4448

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/en/3.0/nightly/contributing/pull-request-walkthrough.html

rochacbruno

test_multiple_plugins.py is a general name, it is not explicit which kind of test is being done on that module. I suggest renaming to

test_sync_multiple_plugins.py
or
test_sync_from_multiple_plugins.py

codecov · 2019-03-22T14:51:14Z

Codecov Report

Merging #42 into master will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   65.63%   65.73%   +0.09%     
==========================================
  Files          65       65              
  Lines        3009     3009              
==========================================
+ Hits         1975     1978       +3     
+ Misses       1034     1031       -3

Impacted Files	Coverage Δ
pulpcore/content/handler.py	`55.36% <0%> (+1.69%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8552476...ad303f7. Read the comment docs.

ragabala · 2019-03-25T14:34:06Z

Hi @rochacbruno ,

Have made those changes. Let me know if this is fine.

Thanks!

rochacbruno · 2019-03-25T15:43:20Z

pulpcore/tests/functional/api/using_plugin/test_sync_multiple_plugins.py

+        # Step 2
+        rpm_remote_body = gen_remote(url=RPM_UNSIGNED_FIXTURE_URL)
+        rpm_remote = self.client.post(RPM_REMOTE_PATH, rpm_remote_body)
+        self.addCleanup(self.client.delete, rpm_remote['_href'])


Not required, just a suggestion, I would avoid unnecessary memory allocation here.

Instead of

rpm_remote_body = gen_remote(url=RPM_UNSIGNED_FIXTURE_URL) rpm_remote = self.client.post(RPM_REMOTE_PATH, rpm_remote_body)

I would do

rpm_remote = self.client.post( RPM_REMOTE_PATH, gen_remote(url=RPM_UNSIGNED_FIXTURE_URL) )

Because that _body variable is used just in one place.

rochacbruno · 2019-03-25T15:43:48Z

pulpcore/tests/functional/api/using_plugin/test_sync_multiple_plugins.py

+        rpm_remote = self.client.post(RPM_REMOTE_PATH, rpm_remote_body)
+        self.addCleanup(self.client.delete, rpm_remote['_href'])
+
+        file_remote_body = gen_remote(url=FILE_FIXTURE_MANIFEST_URL)


same suggestion

goosemania · 2019-03-25T18:33:08Z

FWIW, mirror mode is not supported yet in RPM plugin. So the order of syncs in the test matters.

dralley · 2019-03-26T15:18:38Z

pulpcore/tests/functional/api/using_plugin/constants.py

@@ -65,3 +65,30 @@

 FILE2_URL = urljoin(FILE2_FIXTURE_URL, '1.iso')
 """The URL to an ISO file at :data:`FILE2_FIXTURE_URL`."""
+


Depending on how many future tests will use the RPM plugin from Pulpcore, it might be worth re-evaluating the general strategy being used with the constants, e.g. import them directly from the plugin instead of having independent copies. There's benefits and drawbacks either way but now that a second plugin is being added to these tests it's a good thing to think about.

@dralley +1 for importing the existing constants from plugins. I already did that on Pulp-Certguard test case.

Reason: If constant change on the other plugin it changes/break here.

Long term plans

In long-term we need to re-evaluate all the way Pulp-Smash deals with constants, my suggestion will be to get rid of API and URL paths in constants.

Create some more O.O based approach to have pulp instances and fixtures urls available from pulp-smash across all the plugins tests.

example:

from pulp_smash.entities import Repository, Remote from pulp_smash.fixtures import Fixture repo = Repository() repo.sync(remote=Remote(url=Fixture.FIXTURE_NAME))

On the above example all the classes on .entities already knows its own endpoints no need to pass in constant located urls.

And the Fixtures is not a class, it is a runtime generated ENUM containing entries for each of the pulp-fixtures URLS.

Borderline cases will still need constants, but that should be the minimum.

This effort is tracked on pulp/pulp-smash#1182 and probably will be a Pulp3 only.

daviddavis · 2019-03-27T21:48:24Z

pulpcore/tests/functional/api/using_plugin/test_sync_multiple_plugins.py

+        cls.client = api.Client(cls.cfg, api.json_handler)
+        cls.travis = 'TRAVIS' in os.environ
+
+    @skip_if(bool, 'travis', True)


What happens if I run the functional test suite locally but I don't have the rpm plugin installed? Seems like we should skipping this test on the basis of whether rpm is installed.

Hi @kersommoura @rochacbruno, Are we skipping any tests in any of the plugins depending on the availability of the plugin itself. Else, any idea on how to implement this. I guess this functionality should be implemented in pulp-smash.

Please, Let me know you'r comments.

@ragabala, yes.

See:

pulpcore/pulpcore/tests/functional/api/using_plugin/utils.py

Line 25 in 98bc7b2

def set_up_module():

We can add another function to verify the RPM plugin as well.

ragabala · 2019-04-04T15:08:19Z

Hi Team,

The above issues have been addressed.
Currently, the tests check whether the required plugins are installed before running using the require_pulp_plugins method. Also the constants are pulled from other plugins, however, we aren't checking if the constants are pulled if the other plugin is present in the python environment. This will be addressed in the future, where we move the constants to Pulp Smash.

daviddavis · 2019-04-04T16:06:42Z

Looks like you have some flake8 errors. Also, can you remove the Travis-specific bits now that you are checking for the RPM plugin?

Other than that, LGTM.

This commit will test cross plugin scenario. If the user syncs multiple plugin contents into the same repo with the option mirror=True, the existing content should be overwritten. closes pulp#4448

daviddavis · 2019-04-05T18:26:38Z

Thank you @ragabala! 🎉👯‍♂️👍

ragabala requested a review from a team March 20, 2019 17:23

ragabala force-pushed the fix_4448 branch 3 times, most recently from a362065 to 779425a Compare March 20, 2019 18:45

rochacbruno reviewed Mar 21, 2019

View reviewed changes

ragabala force-pushed the fix_4448 branch from 779425a to 45838fa Compare March 22, 2019 14:38

rochacbruno reviewed Mar 25, 2019

View reviewed changes

rochacbruno approved these changes Mar 25, 2019

View reviewed changes

dralley reviewed Mar 26, 2019

View reviewed changes

daviddavis reviewed Mar 27, 2019

View reviewed changes

ragabala force-pushed the fix_4448 branch 4 times, most recently from 149036b to a1f9715 Compare April 2, 2019 16:40

ragabala force-pushed the fix_4448 branch from a1f9715 to e5de542 Compare April 5, 2019 14:09

Adding mirror true check for rpm/file plugin

ad303f7

This commit will test cross plugin scenario. If the user syncs multiple plugin contents into the same repo with the option mirror=True, the existing content should be overwritten. closes pulp#4448

ragabala force-pushed the fix_4448 branch from e5de542 to ad303f7 Compare April 5, 2019 14:24

nixocio approved these changes Apr 5, 2019

View reviewed changes

nixocio merged commit 7faa5ea into pulp:master Apr 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding mirror true check for rpm/file plugin #42

Adding mirror true check for rpm/file plugin #42

ragabala commented Mar 20, 2019

rochacbruno left a comment

codecov bot commented Mar 22, 2019 •

edited

Loading

ragabala commented Mar 25, 2019

rochacbruno Mar 25, 2019

rochacbruno Mar 25, 2019

goosemania commented Mar 25, 2019

dralley Mar 26, 2019

rochacbruno Mar 26, 2019

daviddavis Mar 27, 2019

ragabala Apr 2, 2019

nixocio Apr 2, 2019

ragabala commented Apr 4, 2019

daviddavis commented Apr 4, 2019

daviddavis commented Apr 5, 2019

		@@ -65,3 +65,30 @@

		FILE2_URL = urljoin(FILE2_FIXTURE_URL, '1.iso')
		"""The URL to an ISO file at :data:`FILE2_FIXTURE_URL`."""

Adding mirror true check for rpm/file plugin #42

Adding mirror true check for rpm/file plugin #42

Conversation

ragabala commented Mar 20, 2019

rochacbruno left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2019 • edited Loading

Codecov Report

ragabala commented Mar 25, 2019

rochacbruno Mar 25, 2019

Choose a reason for hiding this comment

rochacbruno Mar 25, 2019

Choose a reason for hiding this comment

goosemania commented Mar 25, 2019

dralley Mar 26, 2019

Choose a reason for hiding this comment

rochacbruno Mar 26, 2019

Choose a reason for hiding this comment

daviddavis Mar 27, 2019

Choose a reason for hiding this comment

ragabala Apr 2, 2019

Choose a reason for hiding this comment

nixocio Apr 2, 2019

Choose a reason for hiding this comment

ragabala commented Apr 4, 2019

daviddavis commented Apr 4, 2019

daviddavis commented Apr 5, 2019

codecov bot commented Mar 22, 2019 •

edited

Loading