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

#2637 and #878 #18

Merged
merged 16 commits into from May 17, 2017
Merged

#2637 and #878 #18

merged 16 commits into from May 17, 2017

Conversation

mibanescu
Copy link
Member

No description provided.

@pulpbot
Copy link
Member

pulpbot commented Apr 6, 2017

Can one of the admins verify this patch?

1 similar comment
@pulpbot
Copy link
Member

pulpbot commented Apr 6, 2017

Can one of the admins verify this patch?

@@ -0,0 +1,143 @@
# Copyright (c) 2012 Red Hat, Inc.
#
# This software is licensed to you under the GNU General Public
Copy link
Member

Choose a reason for hiding this comment

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

FYI we've been removing these header license blocks from our files. We've been informed that having one license file at the top is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a problem - I will need to do any suggested changes as a different commit, since we use git internally and it won't let me force push. Just FYI :-)

Copy link
Member

Choose a reason for hiding this comment

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

Cool that makes perfect sense. Additional commits are no problem.

REL_FIELDS = ['breaks', 'conflicts', 'depends', 'enhances', 'pre_depends',
'provides', 'recommends', 'replaces', 'suggests']

breaks = mongoengine.DynamicField()
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way these DynamicFields could become more strongly typed? The strong data validation we get with Mongoengine isn't as strong when these fields can take on a variety of data types. I don't know enough about what is in them, but I wanted to ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I've chosen to represent the deps, they are either a list of dict or a list of (list of dict) (in the case of a dependency like foo OR bar).
I believe I tried ListField and it failed for a different reason - I had a duplicated field. If you would rather have it as a ListField, I can do that, as long as each item of the list can be either a dict or a list of dicts.

@bmbouter
Copy link
Member

bmbouter commented Apr 7, 2017

ok test

@bmbouter
Copy link
Member

bmbouter commented Apr 7, 2017

Our Jenkins results are not yet viewable by the community but @dkliban is actively working on that. For now I can say that the PR test runner is failing early on with:

19:53:04 Traceback (most recent call last):
19:53:04   File "./run-tests.py", line 8, in <module>
19:53:04     from pulp.devel.test_runner import run_tests

How are you running your tests locally?

@mibanescu
Copy link
Member Author

Everything has been coded against pulp 2.10.3 (I believe I copied run-tests.py from pulp_rpm from the 2.10-release branch). However, I see that import line to be the same in master. Am I missing a dependency to pull in pulp.devel?

@dkliban
Copy link
Member

dkliban commented Apr 11, 2017

ok test

Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

You need to bring back pulp-dev.py so that this plugin can be installed for development. This script is also used by Jenkins when running unit tests.

Also updated test_requirements.txt and dist_list.txt

Change-Id: I195703d7bd61eab38f95d2e6a7955e5a92aa6fa7
Change-Id: Ic89510ba9c279158c0a94e5042c846df5882ae0f
@dkliban
Copy link
Member

dkliban commented Apr 12, 2017

ok test

pulp-deb.spec Outdated
# ---- Plugins -----------------------------------------------------------------
%package plugins
Summary: Pulp Debian plugins
Group: Development/Languages
Requires: python-pulp-common >= %{pulp_version}
Requires: python-pulp-deb-common >= %{version}
Requires: pulp-server >= %{pulp_version}
Copy link
Member

Choose a reason for hiding this comment

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

You need to keep the version of pulp-server specified so that the plugin is installed with a new enough version of the platform. The unit test jenkins job also relies on this information in the spec file to know which platform bits to install before testing the plugin.

@dkliban
Copy link
Member

dkliban commented Apr 12, 2017

ok test

@mibanescu
Copy link
Member Author

ok test

@mibanescu
Copy link
Member Author

ok test

@mibanescu
Copy link
Member Author

ok test

@mibanescu
Copy link
Member Author

ok test

@bmbouter
Copy link
Member

When you run the tests locally do you also experience the error?

@mock.patch("pulp_deb.plugins.distributors.distributor.aptrepo.debpkg.debfile.DebFile")
@mock.patch("pulp.server.managers.repo._common.task.current")
@mock.patch('pulp.plugins.util.publish_step.repo_controller')
def test_publish_repo(self, _repo_controller, _task_current, _DebFile,
Copy link
Member

Choose a reason for hiding this comment

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

This is a very elaborate test. I was hoping to find an example in the RPM plugin that would help mocking the SELinux restorecon statement, but we don't have one.

Patching the restorecon statement should get you further.

@mock.patch('pulp.plugins.util.publish_step.selinux.restorecon')

@mibanescu
Copy link
Member Author

ok test

@mibanescu
Copy link
Member Author

The tests succeeded on RHEL7. Not sure how to make them run on f25 (I don't think pulp 2.10 was running on it) or f24 (I think it's not finding the repo, for some reason).

@dkliban
Copy link
Member

dkliban commented May 12, 2017

@mibanescu This is so great! @goosemania has already filed an issue for the problem you are experiencing with the unit tests. https://pulp.plan.io/issues/2751 We hope to address it soon. You can consider the unit tests as passing at this point.

@@ -0,0 +1,93 @@
# This software is licensed to you under the GNU General Public
Copy link
Member

Choose a reason for hiding this comment

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

This license block can be removed.

@@ -0,0 +1,102 @@
# -*- coding: utf-8 -*-
#
# This software is licensed to you under the GNU General Public
Copy link
Member

Choose a reason for hiding this comment

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

This license block can be removed.

@@ -0,0 +1,53 @@
# This software is licensed to you under the GNU General Public
Copy link
Member

Choose a reason for hiding this comment

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

This license block can be removed.

@@ -0,0 +1,200 @@
# This software is licensed to you under the GNU General Public
Copy link
Member

Choose a reason for hiding this comment

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

All the license blocks can be removed. Since these are all the same I'll stop leaving the same comment over and over.

dist_list.txt Outdated
@@ -1 +1 @@
el6 el7 fc21 fc22
el6 el7 fc24
Copy link
Member

Choose a reason for hiding this comment

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

Can this be updated to the current support matrix? I believe that would be el7 fc24 fc25.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mibanescu mibanescu force-pushed the master branch 2 times, most recently from 215d7f8 to f417a0e Compare May 15, 2017 19:28
@mibanescu
Copy link
Member Author

ok test

…nstalls

pulp-dev.py can now install packages via pip
test/setup.py added just so pip can be installed.
pulp_rpm is no longer a dependency.
Removed copyright statements.

Change-Id: I8b6954f62fa635335a57f4fedaeaa543b45f16f3
@mibanescu
Copy link
Member Author

ok test

@mibanescu
Copy link
Member Author

What are the next steps for this PR?

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

@mibanescu Thank you so much for this PR. You've implemented all of the changes I requested. The unit tests are all passing. The last thing we need is a sign off from @dkliban.

Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

Thank you!

@dkliban dkliban merged commit de4c554 into pulp:master May 17, 2017
@mibanescu
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants