Fixes #32802 - Sync/Index Python repo with remote options #9437

rverdile · 2021-06-30T19:17:11Z

Create a python repository via a POST request to the API in order to test with remote options. The UI for this is not implemented yet. Syncing through the UI is fine, however.

Include remote options as part of the data field. For example:

"generic_remote_options": { "includes":["[name of python libraries to include from url]"] }

If using https://pypi.org as the url without specifying includes or excludes in generic_remote_options, all of pypi is synced during syncing.

In the console, use ::Katello::RepositoryGenericContentUnit.all to see all the created generic content units. For a given repository repo, repo.generic_content_units will show the content units for that specific repository.

theforeman-bot · 2021-06-30T19:17:33Z

Issues: #32802

ianballou

Some comments from my first read-through:

app/models/katello/generic_content_unit.rb

app/services/katello/pulp3/generic_content_unit.rb

app/services/katello/pulp3/repository/generic.rb

app/models/katello/repository.rb

app/models/katello/concerns/pulp_database_unit.rb

ianballou

This is looking a lot cleaner! I'm going to start testing.

ianballou · 2021-07-07T14:33:01Z

app/services/katello/pulp3/repository/generic.rb

+          repo.update!(:remote_href => response.pulp_href)
+        end
+
+        def refresh_distributions


I still wonder if we could reuse more refresh_distributions code from ::Katello::Pulp3::Repository. I'll leave that to the next reviewer's opinion.

oh sorry i missed this comment, y eah this seem nearly identical to the one there, are there any differences? if not, let just delete this method. If there are differences, maybe a small followup pr is fine

@jlsherrill i believe there is one difference, on line 69, api.class.client_module(repo.repository_type)

ah i see, i'm fine this for now, i think we could improve it in the future though

@jlsherrill @ianballou I'm happy to fix this, but I'm not sure what the syntax would be for a conditional argument error here. I'm assuming the alternative is to have a conditional for generic in repository.rb instead.

ianballou

Found some more out-of-place references to "content":

ianballou · 2021-07-07T15:55:31Z

app/models/katello/repository.rb

@@ -29,6 +29,7 @@ class Repository < Katello::Model
    DOCKER_TYPE = 'docker'.freeze
    OSTREE_TYPE = 'ostree'.freeze
    ANSIBLE_COLLECTION_TYPE = 'ansible_collection'.freeze
+    GENERIC_CONTENT_TYPE = 'generic'.freeze


This should be GENERIC_TYPE

ianballou · 2021-07-07T15:55:45Z

app/models/katello/repository.rb

@@ -130,6 +134,7 @@ class Repository < Katello::Model
    scope :docker_type, -> { with_type(DOCKER_TYPE) }
    scope :ostree_type, -> { with_type(OSTREE_TYPE) }
    scope :ansible_collection_type, -> { with_type(ANSIBLE_COLLECTION_TYPE) }
+    scope :generic_content_type, -> { with_type(GENERIC_CONTENT_TYPE) }


This should be :generic_type.

I also just realized that with_type() is returning [] becuase the Python repo's content_type is 'python' instead of 'generic'. I think we could edit the scope with some kind of query for all generic repositories without using with_type.

I also just realized that with_type() is returning [] becuase the Python repo's content_type is 'python' instead of 'generic'. I think we could edit the scope with some kind of query for all generic repositories without using with_type.

I tried something, not sure about it though. Seems okay if I compare it to the result of with_type on other types.

ianballou

I think something weird is going on with your generic_remote_options parsing:

 pry(main)> ::Katello::Repository.first.root.update(generic_remote_options: {includes: ["pulpcore"]})

[15] pry(main)> ::ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::Repository::Update, ::Katello::Repository.first, SmartProxy.pulp_primary)
2021-07-07T16:44:36 [I|bac|] Task {label: Actions::Pulp3::Orchestration::Repository::Update, id: d74f7328-391c-40ff-b6dc-db435714c4b2, execution_plan_id: da0fb144-7be5-4973-845b-efaa27d54e2f} state changed: planning 
2021-07-07T16:44:36 [I|bac|] Task {label: Actions::Pulp3::Orchestration::Repository::Update, id: d74f7328-391c-40ff-b6dc-db435714c4b2, execution_plan_id: da0fb144-7be5-4973-845b-efaa27d54e2f} state changed: planned 
ForemanTasks::TaskError: Task d74f7328-391c-40ff-b6dc-db435714c4b2: JSON::ParserError: 809: unexpected token at '{:includes:["pulpcore"]}'
from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-4.1.2/lib/foreman_tasks.rb:59:in `block in sync_task

This works, however:

[16] pry(main)> ::Katello::Repository.first.root.update(generic_remote_options: {"includes" => ["pulpcore"]})

ianballou

Looks like name is always nil for me:

#<Katello::GenericContentUnit:0x0000000013dea158 id: 46, name: nil, version: "3.3.1", pulp_id: "/pulp/api/v3/content/python/packages/716012b7-eb41-4f4c-80d2-6f63d9cdeb9c/", content_type: "python_package", created_at: Wed, 07 Jul 2021 16:48:10 UTC +00:00, updated_at: Wed, 07 Jul 2021 16:48:10 UTC +00:00>,

rverdile · 2021-07-07T17:12:45Z

I think something weird is going on with your generic_remote_options parsing:

 pry(main)> ::Katello::Repository.first.root.update(generic_remote_options: {includes: ["pulpcore"]})

[15] pry(main)> ::ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::Repository::Update, ::Katello::Repository.first, SmartProxy.pulp_primary)
2021-07-07T16:44:36 [I|bac|] Task {label: Actions::Pulp3::Orchestration::Repository::Update, id: d74f7328-391c-40ff-b6dc-db435714c4b2, execution_plan_id: da0fb144-7be5-4973-845b-efaa27d54e2f} state changed: planning 
2021-07-07T16:44:36 [I|bac|] Task {label: Actions::Pulp3::Orchestration::Repository::Update, id: d74f7328-391c-40ff-b6dc-db435714c4b2, execution_plan_id: da0fb144-7be5-4973-845b-efaa27d54e2f} state changed: planned 
ForemanTasks::TaskError: Task d74f7328-391c-40ff-b6dc-db435714c4b2: JSON::ParserError: 809: unexpected token at '{:includes:["pulpcore"]}'
from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-4.1.2/lib/foreman_tasks.rb:59:in `block in sync_task

This works, however:

[16] pry(main)> ::Katello::Repository.first.root.update(generic_remote_options: {"includes" => ["pulpcore"]})

So if I make a POST request to the API, the string stored in root.generic_remote_options leaves off the leading " : ". For example,

"{includes => ["pulpcore"]}" POST request
"{:includes => ["pulpcore"]}" The way you show

I suppose I could manually check for the leading " : " and parse accordingly, but is there a cleaner way of parsing?

rverdile · 2021-07-07T17:16:43Z

Looks like name is always nil for me:

#<Katello::GenericContentUnit:0x0000000013dea158 id: 46, name: nil, version: "3.3.1", pulp_id: "/pulp/api/v3/content/python/packages/716012b7-eb41-4f4c-80d2-6f63d9cdeb9c/", content_type: "python_package", created_at: Wed, 07 Jul 2021 16:48:10 UTC +00:00, updated_at: Wed, 07 Jul 2021 16:48:10 UTC +00:00>,

name is a metadata attribute that seems to be determined differently depending on the content type. It's in the generic mockup, but I'm not sure if there's a way to generically include it.

For example,

python simply as a 'name' key to grab the name of the package.
file has a 'relative_path' key that has to be parsed to get the name of the package.

ianballou · 2021-07-07T17:26:46Z

I suppose I could manually check for the leading " : " and parse accordingly, but is there a cleaner way of parsing?

I gave generic_remote_options a ruby hash as the string hash. Looks like it ran a to_s on it in the background. Don't worry about covering that case, I forgot it was stored as a string and not a hash.

I'd say we at least make sure that whatever gets stored for the generic remote options is proper JSON. The example you gave ("{includes => ["pulpcore"]}") isn't proper JSON and it isn't a ruby hash either.

What we should probably do is convert the generic_remote_options to JSON as it comes in from the API.

ianballou · 2021-07-07T17:29:23Z

name is a metadata attribute that seems to be determined differently depending on the content type. It's in the generic mockup, but I'm not sure if there's a way to generically include it.

We might need to add that to the repository type registration file then. We do need a name to go with the version at least. Maybe an array of keys that should be indexed with the content units?

rverdile · 2021-07-12T14:29:59Z

@jlsherrill could you review this when you have a chance?

jlsherrill · 2021-07-12T14:55:47Z

app/services/katello/pulp3/generic_content_unit.rb

+        custom_json['pulp_id'] = backend_data['pulp_href']
+        custom_json['name'] = repository_type.model_name.call(backend_data)
+        custom_json['version'] = repository_type.model_version.call(backend_data)
+        custom_json['content_type'] = repository_type.content_types.first.content_type


This '.first' doesn't seem right and wouldn't work properly if there were more than one type. I think the content_type needs to be passed in somehow

jlsherrill · 2021-07-12T14:58:31Z

app/services/katello/pulp3/generic_content_unit.rb

+      end
+
+      def self.content_api(repository_type)
+        repository_type.content_types.first.pulp3_api.new(repository_type.pulp3_api_class.new(SmartProxy.pulp_primary!, repository_type).api_client)


ah i see the .first here again. If we want to proceed with this, maybe lets open up an issue to fix this and put it on the board.

@jlsherrill could this be used to get the content_type from within the function ::Katello::RepositoryTypeManager.find(repository.content_type).default_managed_content_type.content_type (assuming repository is passed instead of repository_type)? I'm not sure if this works if there's more than one content type.

Nevermind. That works for "default" :)

ianballou · 2021-07-12T20:05:50Z

app/services/katello/repository_type.rb

+      end
+    end
+
+    class RepositoryRemoteOptions


RepositoryRemoteOptions confused me a little because usually we keep a prefix like Repository for joins tables (like RepositoryErratum). Also the class should be singular since it represents a single remote option. Maybe GenericRemoteOption?

ianballou

Here are my comments for getting the remote options working:

ianballou · 2021-07-12T21:35:56Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -473,6 +483,14 @@ def construct_repo_from_params(repo_params) # rubocop:disable Metrics/AbcSize
                                                            :checksum_type, :download_policy, :http_proxy_policy).to_h.with_indifferent_access)
      root.docker_upstream_name = repo_params[:docker_upstream_name] if repo_params[:docker_upstream_name]
      root.docker_tags_whitelist = repo_params.fetch(:docker_tags_whitelist, []) if root.docker?
+      root.generic_remote_options = repo_params.fetch(:generic_remote_options, []).to_json if root.generic?


This can be deleted.

ianballou · 2021-07-12T21:37:49Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -43,6 +43,11 @@ def custom_index_relation(collection)
      param :checksum_type, String, :desc => N_("Checksum of the repository, currently 'sha1' & 'sha256' are supported")
      param :docker_upstream_name, String, :desc => N_("Name of the upstream docker repository")
      param :docker_tags_whitelist, Array, :desc => N_("Comma-separated list of tags to sync for Container Image repository")
+      RepositoryTypeManager.repository_types.each do |_, type|


The params aren't showing up under "repository" because they aren't being wrapped above at this line:

wrap_parameters :repository, :include => RootRepository.attribute_names.concat([:ignore_global_proxy])

So, we need to replace that line above with something like the following:

generic_repo_wrap_params = [] RepositoryTypeManager.repository_types.each do |_, type| type.repository_remote_options.each do |option| generic_repo_wrap_params << option.name end end repo_wrap_params = RootRepository.attribute_names.concat([:ignore_global_proxy]) + generic_repo_wrap_params wrap_parameters :repository, :include => repo_wrap_params

ianballou · 2021-07-12T21:38:36Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -446,6 +451,11 @@ def repository_params
             ]

      keys += [{:docker_tags_whitelist => []}, :docker_upstream_name] if params[:action] == 'create' || @repository&.docker?
+      RepositoryTypeManager.repository_types.each do |_, type|
+        type.repository_remote_options.each do |option|
+          keys += [option.name] if params[:action] == 'create' || @repository&.generic?


This worked for me:

keys += [{option.name => []}] if params[:action] == 'create' || @repository&.generic?

...but of course you'll need to edit based on the remote option type, so you have string and arrays

ianballou · 2021-07-19T22:37:24Z

I'll get to this as soon as I can tomorrow!

ianballou

I found an issue with parsing of the remote options. Here's what I did:

$ curl --user admin:changeme --header "Content-Type: application/json"  --request POST --data '{"product_id":"1","content_type":"python","name":"test9","url":"https://pypi.org","includes":["pulpcore"]}' https://`hostname`/katello/api/repositories/

$ curl --user admin:changeme --header "Content-Type: application/json"  --request PUT --data '{"includes":["pulp-rpm-client"]}' https://`hostname`/katello/api/repositories/54

Then I synced the repo. This gave the following error:

Action:

Actions::Pulp3::Repository::RefreshRemote

Input:

{"repository_id"=>54,
 "smart_proxy_id"=>1,
 "remote_user"=>"admin",
 "remote_cp_user"=>"admin",
 "locale"=>"en",
 "current_request_id"=>"0990ea9f-8153-4ab3-9140-6fc0d730f148",
 "current_timezone"=>"America/New_York",
 "current_organization_id"=>1,
 "current_location_id"=>2,
 "current_user_id"=>4}

Output:

{}

Exception:

JSON::ParserError: 809: unexpected token at '{"excludes"=>nil, "includes"=>["pulp-rpm-client"], "package_types"=>nil}'

Backtrace:

/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/json-2.5.1/lib/json/common.rb:216:in `parse'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/json-2.5.1/lib/json/common.rb:216:in `parse'
/home/vagrant/katello/app/services/katello/pulp3/repository/generic.rb:18:in `remote_options'
/home/vagrant/katello/app/services/katello/pulp3/repository.rb:172:in `compute_remote_options'
/home/vagrant/katello/app/services/katello/pulp3/repository.rb:147:in `remote_needs_updates?'
/home/vagrant/katello/app/lib/actions/pulp3/repository/refresh_remote.rb:12:in `invoke_external_task'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action/polling.rb:84:in `initiate_external_action'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action/polling.rb:19:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action/cancellable.rb:14:in `run'
/home/vagrant/katello/app/lib/actions/pulp3/abstract_async_task.rb:10:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:572:in `block (3 levels) in execute_run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:32:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/katello/app/lib/actions/middleware/remote_action.rb:16:in `block in run'
/home/vagrant/katello/app/lib/actions/middleware/remote_action.rb:40:in `block in as_remote_user'
/home/vagrant/katello/app/models/katello/concerns/user_extensions.rb:21:in `cp_config'
/home/vagrant/katello/app/lib/actions/middleware/remote_action.rb:27:in `as_cp_user'
/home/vagrant/katello/app/lib/actions/middleware/remote_action.rb:39:in `as_remote_user'
/home/vagrant/katello/app/lib/actions/middleware/remote_action.rb:16:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/rails_executor_wrap.rb:14:in `block in run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.0.3.7/lib/active_support/execution_wrapper.rb:88:in `wrap'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/rails_executor_wrap.rb:13:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action/progress.rb:31:in `with_progress_calculation'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action/progress.rb:17:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/katello/app/lib/actions/middleware/keep_locale.rb:11:in `block in run'
/home/vagrant/katello/app/lib/actions/middleware/keep_locale.rb:22:in `with_locale'
/home/vagrant/katello/app/lib/actions/middleware/keep_locale.rb:11:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/load_setting_values.rb:20:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_request_id.rb:15:in `block in run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_request_id.rb:52:in `restore_current_request_id'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_request_id.rb:15:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_timezone.rb:15:in `block in run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_timezone.rb:44:in `restore_curent_timezone'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_timezone.rb:15:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_taxonomies.rb:15:in `block in run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_taxonomies.rb:45:in `restore_current_taxonomies'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_taxonomies.rb:15:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:32:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:27:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware.rb:19:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_user.rb:15:in `block in run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_user.rb:54:in `restore_curent_user'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-5.0.0/app/lib/actions/middleware/keep_current_user.rb:15:in `run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/stack.rb:23:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/middleware/world.rb:31:in `execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:571:in `block (2 levels) in execute_run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:570:in `catch'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:570:in `block in execute_run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:473:in `block in with_error_handling'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:473:in `catch'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:473:in `with_error_handling'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:565:in `execute_run'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/action.rb:286:in `execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/execution_plan/steps/abstract_flow_step.rb:18:in `block (2 levels) in execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/execution_plan/steps/abstract.rb:167:in `with_meta_calculation'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/execution_plan/steps/abstract_flow_step.rb:17:in `block in execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/execution_plan/steps/abstract_flow_step.rb:32:in `open_action'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/execution_plan/steps/abstract_flow_step.rb:16:in `execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/director.rb:69:in `execute'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/executors/parallel/worker.rb:15:in `block in on_message'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/executors.rb:18:in `run_user_code'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/executors/parallel/worker.rb:14:in `on_message'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/context.rb:46:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/executes_context.rb:7:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/actor.rb:122:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/awaits.rb:15:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/sets_results.rb:14:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/dynflow-1.5.0/lib/dynflow/actor.rb:56:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/buffer.rb:38:in `process_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/buffer.rb:31:in `process_envelopes?'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/buffer.rb:20:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/termination.rb:55:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/removes_child.rb:10:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/abstract.rb:25:in `pass'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/behaviour/sets_results.rb:14:in `on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/core.rb:162:in `process_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/core.rb:96:in `block in on_envelope'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/core.rb:119:in `block (2 levels) in schedule_execution'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/synchronization/mutex_lockable_object.rb:47:in `block in synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/synchronization/mutex_lockable_object.rb:47:in `synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/synchronization/mutex_lockable_object.rb:47:in `synchronize'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-edge-0.6.0/lib/concurrent-ruby-edge/concurrent/actor/core.rb:116:in `block in schedule_execution'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/serialized_execution.rb:18:in `call'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/serialized_execution.rb:96:in `work'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/serialized_execution.rb:77:in `block in call_job'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:363:in `run_task'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:352:in `block (3 levels) in create_worker'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:335:in `loop'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:335:in `block (2 levels) in create_worker'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:334:in `catch'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb:334:in `block in create_worker'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/logging-2.3.0/lib/logging/diagnostic_context.rb:474:in `block in create_with_logging_context'

ianballou · 2021-07-20T16:37:42Z

app/controllers/katello/api/v2/repositories_controller.rb

    def construct_repo_from_params(repo_params) # rubocop:disable Metrics/AbcSize
      root = @product.add_repo(repo_params.slice(:label, :name, :description, :url, :content_type, :arch, :unprotected,
-                                                            :gpg_key, :ssl_ca_cert, :ssl_client_cert, :ssl_client_key,
-                                                            :checksum_type, :download_policy, :http_proxy_policy).to_h.with_indifferent_access)


Looks like a formatting-only change

ianballou · 2021-07-20T16:54:21Z

Looks like the generic remote options weren't saved as proper JSON after the update:

[7] pry(main)> ::Katello::Repository.generic_type.last.root.generic_remote_options
=> "{\"excludes\"=>nil, \"includes\"=>[\"pulp-rpm-client\"], \"package_types\"=>nil}"

ianballou · 2021-07-20T17:41:40Z

app/services/katello/repository_type_manager.rb

@@ -140,6 +140,14 @@ def enabled?(repository_type)
        find(repository_type).present?
      end

+      def generic_remote_options
+        options = []
+        enabled_repository_types.each do |_, type|


I'm working on an issue right now where params in controllers shouldn't be based on enabled repo types. It's because, at rpm build time, when the api cache is generated, the enabled types will be empty. I would give an argument for enabled_only (def generic_remote_options(enabled_only = true). Then, if enabled_only is false, use defined_repository_types instead of enabled_repository_types. That way, you can still use generic_remote_options for cases where you want to use the enabled types (which should be pretty much any case other than when you're creating the controller params).

Example: https://github.com/Katello/katello/pull/9480/files#diff-9d7db34467c739b369c12749ec9e0f66c590619691fb5921a6f6ef42f201a8bdR48

app/controllers/katello/api/v2/repositories_controller.rb

ianballou · 2021-07-20T19:06:31Z

app/controllers/katello/api/v2/repositories_controller.rb

+
+    def generic_remote_options_hash(repo_params)
+      generic_remote_options = {}
+      RepositoryTypeManager.generic_remote_options(repository_type: @repository.repository_type).each do |option|


@repository isn't defined yet if you're creating a repo.

ianballou

I was able to CRUD repositories successfully and sync them using various remote options. I think this is good! Just one more comment:

ianballou · 2021-07-20T20:08:10Z

app/services/katello/repository_type_manager.rb

@@ -140,6 +140,19 @@ def enabled?(repository_type)
        find(repository_type).present?
      end

+      def generic_remote_options(opts = {})
+        options = []
+        repo_types = opts[:enabled_only] ? enabled_repository_types : defined_repository_types


You might want to make the default use enabled_repository_types if no options are given. I think in most cases you will want to used the enabled types only.

ianballou · 2021-07-20T21:02:31Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -62,6 +69,10 @@ def custom_index_relation(collection)
      param :http_proxy_id, :number, :desc => N_("ID of a HTTP Proxy")
      param :arch, String, :desc => N_("Architecture of content in the repository")
      param :retain_package_versions_count, :number, :desc => N_("The maximum number of versions of each package to keep.")
+
+      RepositoryTypeManager.generic_remote_options.each do |option|


generic_remote_options(defined_only: true)

ianballou · 2021-07-21T14:24:12Z

[test katello]

ianballou

From my testing yesterday this gets my ACK. @jlsherrill we just need yours now.

jlsherrill · 2021-07-22T14:25:25Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -2,7 +2,14 @@ module Katello
  class Api::V2::RepositoriesController < Api::V2::ApiController # rubocop:disable Metrics/ClassLength
    include Katello::Concerns::FilteredAutoCompleteSearch

-    wrap_parameters :repository, :include => RootRepository.attribute_names.concat([:ignore_global_proxy])
+    generic_repo_wrap_params = []
+    RepositoryTypeManager.generic_remote_options.each do |option|


I think this needs to use Defined_only: true as well, as the class gets loaded at startup, so this will be executed only once when the rails app loads

If you agree, i think this is the last thing! Otherwise this looks good to me

@jlsherrill this seemed to work with testing. what would be case where enabled_only being true might because an issue?

made the change, just curious more about what the difference is.

@rverdile one way to test this out would be to clear out the pulp primary smart proxy's capabilities, stick a debugger in the code here, and restart your server. I'm pretty sure you'd see the enabled content types list would be empty by the time this is run. If the smart proxy is corrected with a refresh after that, this class would still be loaded with the incorrect wrap_parameters. The only way to fix it would be to restart your server.

ianballou · 2021-07-22T17:56:19Z

[test katello]

jlsherrill · 2021-07-23T14:14:57Z

[test katello]

jlsherrill · 2021-07-23T18:28:46Z

Thanks @rverdile !! good work!

theforeman-bot added Needs testing Not yet reviewed labels Jun 30, 2021

ianballou reviewed Jul 6, 2021

View reviewed changes

app/models/katello/generic_content_unit.rb Outdated Show resolved Hide resolved

app/services/katello/pulp3/generic_content_unit.rb Outdated Show resolved Hide resolved

app/services/katello/pulp3/repository/generic.rb Outdated Show resolved Hide resolved

rverdile force-pushed the feat#32802 branch 2 times, most recently from 0d1b829 to b7a57d2 Compare July 6, 2021 19:58

ianballou reviewed Jul 6, 2021

View reviewed changes

app/models/katello/repository.rb Outdated Show resolved Hide resolved

app/models/katello/concerns/pulp_database_unit.rb Outdated Show resolved Hide resolved

rverdile force-pushed the feat#32802 branch 4 times, most recently from 58385a7 to 6011e75 Compare July 7, 2021 13:09

ianballou reviewed Jul 7, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch 2 times, most recently from 3832fe2 to f333474 Compare July 12, 2021 14:25

rverdile requested a review from ianballou July 12, 2021 14:30

jlsherrill reviewed Jul 12, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch from f333474 to 04e37a2 Compare July 12, 2021 19:46

ianballou reviewed Jul 12, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch from 04e37a2 to aff61ef Compare July 13, 2021 13:04

rverdile requested a review from ianballou July 13, 2021 13:23

rverdile requested a review from jlsherrill July 19, 2021 13:18

ianballou reviewed Jul 20, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch from 30f4ca2 to ac149b2 Compare July 20, 2021 17:23

ianballou reviewed Jul 20, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch 2 times, most recently from 8e8b0ea to 79f4670 Compare July 20, 2021 18:55

ianballou reviewed Jul 20, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch from 79f4670 to 0499fe4 Compare July 20, 2021 19:32

ianballou reviewed Jul 20, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch from 0499fe4 to 20b68c2 Compare July 20, 2021 20:58

ianballou reviewed Jul 20, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch 2 times, most recently from 6e68d3f to 167b292 Compare July 20, 2021 21:35

ianballou reviewed Jul 21, 2021

View reviewed changes

rverdile force-pushed the feat#32802 branch 3 times, most recently from 6ed61bc to 9e850b3 Compare July 22, 2021 13:21

jlsherrill reviewed Jul 22, 2021

View reviewed changes

Fixes #32802 - Sync/Index Python repo with remote options

7723db9

rverdile force-pushed the feat#32802 branch from 9e850b3 to 7723db9 Compare July 22, 2021 15:34

rverdile requested review from ianballou and jlsherrill July 23, 2021 17:36

jlsherrill approved these changes Jul 23, 2021

View reviewed changes

jlsherrill merged commit 1afd1c5 into Katello:master Jul 23, 2021

Fixes #32802 - Sync/Index Python repo with remote options #9437

Fixes #32802 - Sync/Index Python repo with remote options #9437

Conversation

rverdile commented Jun 30, 2021

theforeman-bot commented Jun 30, 2021

ianballou left a comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rverdile Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment • edited Loading

Choose a reason for hiding this comment

ianballou left a comment • edited Loading

Choose a reason for hiding this comment

rverdile commented Jul 7, 2021

rverdile commented Jul 7, 2021

ianballou commented Jul 7, 2021

ianballou commented Jul 7, 2021

rverdile commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Jul 19, 2021

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Jul 21, 2021

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rverdile Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

ianballou Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

ianballou commented Jul 22, 2021

jlsherrill commented Jul 23, 2021

jlsherrill commented Jul 23, 2021

rverdile Jul 21, 2021 •

edited

Loading

ianballou left a comment •

edited

Loading

ianballou left a comment •

edited

Loading

rverdile Jul 22, 2021 •

edited

Loading

ianballou Jul 22, 2021 •

edited

Loading