-
Notifications
You must be signed in to change notification settings - Fork 83
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
[WIP] Add content-view version export/import skip-rpms options. #718
[WIP] Add content-view version export/import skip-rpms options. #718
Conversation
Hi @jeffmcutter thanks for your PR :) When you are ready to take it out of WIP let me know and I will test and review this for you. |
Hi @chris1984. I'm happy with how the functionality works now, I'm just not too familiar with contributing to this project. Do I need to write specs / tests? I'd be interested in testing and review plus feedback. Should I remove the WIP to start that process? Thanks! |
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.
Thanks for the PR! A test for this new option would be great. Rubocop also has a few complaints, which is causing the build to fail CI:
lib/hammer_cli_katello/content_view_version.rb:352:101: C: Line is too long. [131/100]
option '--skip-rpms', :flag, _("Do not include RPMs in export (content must be synchronized to destination prior to import)")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/hammer_cli_katello/content_view_version.rb:403:34: C: Do not leave space between ! and its argument.
if repositories&.any? && ! options['option_skip_rpms']
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/hammer_cli_katello/content_view_version.rb:450:101: C: Line is too long. [106/100]
option '--skip-rpms', :flag, _("Skip import of RPMs (content must be synchronized prior to import)")
^^^^^^
Hi @akofink, I fixed the Rubocop complaints. I will look at writing tests in a week or two. Thanks, |
@jeffmcutter we are looking to get this into the next release of hammer, I can write unit tests for you and supply you a patch if you want to rebase and push. |
@chris1984 That would be fabulous if you could. I'm sorry I've just not had opportunity to get to it, but I would love to get this in there. Thank you! |
Hmm, looks like that wasn't the right thing to do? I was trying to rebase to get current before attempting to write tests. |
@jeffmcutter hey, so it looks like what you want to do is If you need help with any of the tests let me know |
677b997
to
15e9ccd
Compare
@chris1984 I finally got my developer setup going and current. It's a bit tricky for this since hammer needs to run locally but the forklift stuff sets it up on its own VM, but I glued the two together and am currently testing that the changes behave as expectedly on the current version. I think I will submit a PR with that to forklift. Also encountered some difficulties here theforeman/forklift#1277. Can you suggest some of the sorts of tests I should be thinking about for this change? I've written some tests for a ManageIQ PR before but I'm not yet making sense of https://github.com/Katello/hammer-cli-katello/blob/master/test/functional/content_view/version/export_test.rb and https://github.com/Katello/hammer-cli-katello/blob/master/test/functional/content_view/version/import_test.rb which seems to be the right place for it unless I'm mistaken. Thanks, |
Having difficulties with the content-view version in upstream/master:
|
@jeffmcutter sorry for the delay, let me try to replicate what you are hitting to give you the next steps. |
with -d
|
I created the missing /var/lib/pulp/published/yum/https/repos/ and tried to run the export again in debug mode and got the following error:
In comparing /var/lib/pulp on the current Foreman dev against a Satellite 6.8 server, it appears that /var/lib/pulp has been restructured. Running a find to look for the organization name shows it does not exist on the Foreman dev but does on Satellite 6.8. The actual rpms appear to now be stored under /var/lib/pulp/media/artifact which I do not see on Satellite 6.8.
It appears that hammer content-view version export is broken as a result of these changes. |
I opened an issue: |
@jeffmcutter for the Katello box, I see you are using master. Are you using a katello devel box from vagrant/forklift or a nightly production install from katello.org? |
@chris1984 I am using a katello-devel box from forklift which I have modified to include hammer as you can see in this PR: I'm waiting for clarification, but it seems that content-view version import/export may be deprecated in newer versions of Katello or something along those lines: https://projects.theforeman.org/issues/31735 Thanks, |
That is correct, the fixes you are adding are for Pulp 2 Which is for 3.17 and below. Katello 3.18+ is where Pulp3 started being used 100% of the time. For our downstream releases your fix would affect Satellite 6.9 and below. So we can still keep working on this if you want. |
Being short on time again lately, and this functionality being short-lived (theforeman/forklift#1279). I will close this out. Thank you for your time and efforts. |
Use case:
Two separate Foreman servers which have repositories synchronized from upstream sources need to have the same content-view versions to be able to facilitate consistent life cycle management. For example, one Foreman server may be used for development environments patching and a separate Foreman server may be required to manage production environments patching due to security restrictions.
The new content-view version export/import process works nicely for this, except that if content is synchronized already via a sync plan, the export and import of the RPM repositories can create significant and unnecessary overhead in the export / import / and transfer of the tar ball from the export between export and import systems.
This PR is a work in progress. It is able to export and import content-view versions without including the RPMs using the --skip-rpms option to both content-view version export and import commands. I'm looking for feedback about this idea and implementation.
RFE BZ for this functionality: https://bugzilla.redhat.com/show_bug.cgi?id=1744255