Skip to content

Fixes #38542 - Delay params for import_uploads action #11430

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+25 −10
Open

Fixes #38542 - Delay params for import_uploads action #11430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions app/controllers/katello/api/v2/repositories_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ class Api::V2::RepositoriesController < Api::V2::ApiController # rubocop:disable
generic_repo_wrap_params << option.name
end

repo_wrap_params = RootRepository.attribute_names + generic_repo_wrap_params

wrap_parameters :repository, :include => repo_wrap_params
Comment on lines -10 to -12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not necessary, but seems nicer to delegate atribute_names call to Rails instead of calling it explicitly here. I'd love to get rid of calling database related stuff during initialization time.

wrap_parameters RootRepository, name: :repository, :include => generic_repo_wrap_params

CONTENT_CREDENTIAL_GPG_KEY_TYPE = "gpg_key".freeze
CONTENT_CREDENTIAL_SSL_CA_CERT_TYPE = "ssl_ca_cert".freeze
Expand Down Expand Up @@ -493,7 +491,7 @@ def upload_content
param :async, :bool, desc: N_("Do not wait for the ImportUpload action to finish. Default: false")
param 'publish_repository', :bool, :desc => N_("Whether or not to regenerate the repository on disk. Default: true")
param 'sync_capsule', :bool, :desc => N_("Whether or not to sync an external capsule after upload. Default: true")
param :content_type, RepositoryTypeManager.uploadable_content_types(false).map(&:label), :required => false, :desc => N_("content type ('deb', 'docker_manifest', 'file', 'ostree_ref', 'rpm', 'srpm')")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We introduced callable_enum in Foreman some time ago just for such cases: theforeman/foreman#10494

param :content_type, :callable_enum, of: -> { RepositoryTypeManager.uploadable_content_types(false).map(&:label) }, :required => false, :desc => N_("content type ('deb', 'docker_manifest', 'file', 'ostree_ref', 'rpm', 'srpm')")
param :uploads, Array, :desc => N_("Array of uploads to import") do
param 'id', String, :required => true
param 'content_unit_id', String
Expand All @@ -502,12 +500,6 @@ def upload_content
param 'name', String, :desc => N_("Needs to only be set for file repositories or docker tags"), :required => true
param 'digest', String, :desc => N_("Needs to only be set for docker tags")
end
Katello::RepositoryTypeManager.generic_repository_types.each_pair do |_, repo_type|
repo_type.import_attributes.each do |import_attribute|
param import_attribute.api_param, import_attribute.type,
:desc => N_(import_attribute.description)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking out loud now. Would it be possible to extend the DSL with a method so you still keep the API definition in one place? For example lazy_api (or better naming):

Suggested change
end
lazy_api do
Katello::RepositoryTypeManager.generic_repository_types.each_value do |repo_type|
repo_type.import_attributes.each do |import_attribute|
param import_attribute.api_param, import_attribute.type, desc: import_attribute.description
end
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but this would introduce a new pattern (not that this is bad, but this new method will be available for every controller which use apipie mixin potentially introducing more conflicts).

Current PR to apipie-rails rather extends existing pattern. Personally I don't care, but it's up to maintainers/reviewers/users to decide.

def import_uploads
generate_metadata = ::Foreman::Cast.to_bool(params.fetch(:publish_repository, true))
sync_capsule = ::Foreman::Cast.to_bool(params.fetch(:sync_capsule, true))
Expand Down
20 changes: 20 additions & 0 deletions app/controllers/katello/concerns/api/v2/dynamic_params/repositories.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module Katello
module Concerns
module Api::V2
module DynamicParams
module Repositories
extend ::Apipie::DSL::Concern

lazy_update_api(:import_uploads) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lazy_update_api doesn't exist in apipie-rails yet, the PR is open: Apipie/apipie-rails#955

::Katello::RepositoryTypeManager.generic_repository_types.each_pair do |_, repo_type|
repo_type.import_attributes.each do |import_attribute|
param import_attribute.api_param, import_attribute.type,
:desc => N_(import_attribute.description)
Copy link
Member

Choose a reason for hiding this comment

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

This was already there, but N_() doesn't work with variables so

Suggested change
:desc => N_(import_attribute.description)
:desc => import_attribute.description

end
end
end
end
end
end
end
end
3 changes: 3 additions & 0 deletions config/initializers/dynamic_params.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Rails.application.config.after_initialize do
Katello::Api::V2::RepositoriesController.include Katello::Concerns::Api::V2::DynamicParams::Repositories
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be done now it different places, e.g. in to_prepare block in engine.rb. This is just for readability? and a default place for similar cases.

end
Loading