-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add tests for lazy on-demand syncing for RPM. #117
Conversation
|
||
:param server_config: A :class:`pulp_smash.config.ServerConfig` object. | ||
:param importer: A dict-like object with importer configuration. | ||
:returns: A tuple of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is returned?
Please write a proper commit message. For example commit messages, browse |
|
||
for service in services: | ||
service.start() | ||
|
||
|
||
def reset_squid(server_config): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding this function in a separate commit. It is easier to revert commits if each contains one logical change, among other benefits.
I got the following error when I tried running this PR: `[jenkins@rhel7-vanilla-np-qeos-73966 pulp-smash]$ python -m unittest2 pulp_smash.tests.rpm.api_v2.test_sync_publish .................sE....ERROR: setUpClass (pulp_smash.tests.rpm.api_v2.test_sync_publish.SyncOnDemandTestCase)Traceback (most recent call last): ` |
@dkliban ahh, I rebased and I guess failed to resolve the merge conflicts correctly. |
FYI, as of c6308d4, |
The specific error you're seeing may be a result of 45cadf4, wherein several functions were moved to |
1920f06
to
144f21b
Compare
@Ichimonji10 It's going to be a while before I have time for this. It will probably be much more efficient for everyone involved if you simply take what's here and modify it to fit your needs. Alternatively you can re-review it and it'll be a few days before I address the issues. |
@jeremycline OK, I may do that. Thanks for the status update. 😄 |
I took what is in this pull request and modified it in https://github.com/Ichimonji10/pulp-smash/tree/117. Unfortunately, most of the tests |
@Ichimonji10 Is there something else you're expecting from me? That link is no longer around and I'm not really sure what you mean by 'Unfortunately, most of the tests pass'. |
Last I checked, most of the tests fail. Can you take a look at them and see if they still fail or not? Sorry about the broken link. That was a snafu on my part. The link should be valid again. |
Fix pulp#73. This adds tests for consuming content from a repository that has been configured with the `on_demand` download policy. This also adds a method to reset Squid and its cache, and also fixes the `reset_pulp` method. Test results prior to this commit: Test results after this commit:
144f21b
to
c31c35c
Compare
This test fails for me with the following error:
The interesting part there is the mention of "dev.example.com." That makes me think that this test will succeed when run from within a development Vagrant VM, but not in other contexts, and it would explain the "name or service not known" error I'm receiving. Any insight here? |
@Ichimonji10 Is the Pulp server's hostname I'm guessing some setup automation needs to change to set up the reverse proxy, caching proxy, and streamer for lazy content syncing. The Ansible role we use in the development environment[0] is probably a good starting point, but the Squid configuration file needs to be different for RHEL6[1]. [0] https://github.com/pulp/pulp/tree/master/playpen/ansible/roles/lazy |
Yes. For what it's worth, I'm running the test suite outside of the VM itself. |
Yes, that's expected behavior. Lazy redirects (by default) the the FQDN of the host Pulp is running on. |
@dkliban, do the Pulp VMs used by Jenkins return correct hostnames? @jeremycline, is this feature new in 2.8? |
@Ichimonji10 yes, it's new in 2.8 |
OK. I'll add in appropriate test skipping logic when I return to this tomorrow. |
Merged as cd92cd1 and 036f67d. Thank you, @jeremycline! |
This closes issue #73