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

fixes #21951 - don't allow export of on-demand repos #7338

Merged
merged 1 commit into from May 3, 2018

Conversation

stbenjam
Copy link
Contributor

Prevents exporting content views with on demand repositories:

[root@centos7-devel ~]# hammer -s https://$HOSTNAME content-view version export --id 9
[Foreman] Password for admin: 
Could not export the content view:
  Content views with on-demand repositories cannot be exported.

@theforeman-bot
Copy link

Issues: #21951

@stbenjam
Copy link
Contributor Author

[test katello]

@cfouant cfouant self-assigned this May 1, 2018
@stbenjam stbenjam changed the title fixes #21951 - don't allow export of cv w/ on-demand repos fixes #21951 - don't allow export of on-demand repos May 1, 2018
@stbenjam
Copy link
Contributor Author

stbenjam commented May 1, 2018

Addressed the feedback I got this morning. Individual repositories are handled, and error message now includes the names of which repos are on-demand:

[vagrant@centos7-devel ~]$ hammer -s https://$HOSTNAME -u admin -p changeme repository export --id 1
Could not export the repository:
  On-demand repositories cannot be exported.

[vagrant@centos7-devel ~]$ hammer -s https://$HOSTNAME -u admin -p changeme content-view version export --id 2
Could not export the content view:
  This content view has on-demand repositories that cannot be exported: Zoo

Copy link
Member

@cfouant cfouant left a comment

Choose a reason for hiding this comment

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

Tested this and the code works wonderfully. The error message is much more helpful now. Happy to ACK once minor nitpicks are addressed 👍

@@ -250,6 +250,8 @@ def export

fail HttpErrors::BadRequest, _("Repository content type must be 'yum' to export.") unless @repository.content_type == 'yum'

fail HttpErrors::BadRequest, _("On-demand repositories cannot be exported.") if @repository.download_policy == ::Runcible::Models::YumImporter::DOWNLOAD_ON_DEMAND
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Since hammer/UI refer to this as on demand, can we remove the hyphen?

@@ -82,6 +82,10 @@ def export
fail HttpErrors::BadRequest, _("ISO export must be enabled when specifying ISO size")
end

if (repos = @version.content_view.on_demand_repositories).any?
fail HttpErrors::BadRequest, _("This content view has on-demand repositories that cannot be exported: %{repos}" % {repos: repos.pluck(:label).join(", ")})
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Since hammer/UI refer to this as on demand, can we remove the hyphen?

@stbenjam
Copy link
Contributor Author

stbenjam commented May 2, 2018

@cfouant Thanks for the review! Good call for on demand, didn't notice the hyphen. Updated

@cfouant
Copy link
Member

cfouant commented May 3, 2018

ACK thanks @stbenjam !

@stbenjam stbenjam merged commit 64110fe into Katello:master May 3, 2018
@stbenjam
Copy link
Contributor Author

stbenjam commented May 3, 2018

Thanks @cfouant !

@stbenjam stbenjam deleted the on-demand branch May 3, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants