Fixes #13485 - API: Allow user to set sync type per repository #5749

daviddavis · 2016-02-01T14:28:37Z

cfouant · 2016-02-01T15:52:32Z

app/models/katello/repository.rb

@@ -84,6 +84,8 @@ class Repository < Katello::Model
      :allow_blank => false,
      :message => ->(_, _) { _("must be one of the following: %s") % Katello::RepositoryTypeManager.repository_types.keys.join(', ') }
    }
+    validates :download_policy, inclusion: { in: ::Runcible::Models::Importer::DOWNLOAD_POLICIES }, if: :yum?


I think this needs to be
NameError: uninitialized constant Runcible::Models::YumImporter::DOWNLOAD_POLICIES

parthaa · 2016-02-01T20:50:11Z

I presume tests are getting added..

cfouant · 2016-02-01T21:07:26Z

@parthaa - yes I have a p-r opened against his branch that will add tests and show download_policy for yum repos only.

jlsherrill · 2016-02-02T03:03:00Z

db/migrate/20160131182301_add_download_policy_to_katello_repositories.rb

@@ -0,0 +1,5 @@
+class AddDownloadPolicyToKatelloRepositories < ActiveRecord::Migration
+  def change
+    add_column :katello_repositories, :download_policy, :string


do you want to set a default in the migraiton?

@jlsherrill Probably not if only valid for yum repos, right?

ehelms · 2016-02-03T02:47:17Z

db/migrate/20160131182301_add_download_policy_to_katello_repositories.rb

+class AddDownloadPolicyToKatelloRepositories < ActiveRecord::Migration
+  def change
+    add_column :katello_repositories, :download_policy, :string
+    update "UPDATE katello_repositories SET download_policy = 'on_demand' WHERE content_type = 'yum'"


Could this be done through the ORM? We recently had a similar discussion around this on #5709 (comment)

What would be the advantage here of using ORM?

Prevent database specific sequel from being used and letting the ORM handle things properly for us

That's how we tend to do things everywhere else unless they can't be accomplished through the ORM

daviddavis · 2016-02-03T15:42:29Z

app/views/katello/api/v2/repositories/show.json.rabl

@@ -6,6 +6,9 @@ extends 'katello/api/v2/common/timestamps'
 attributes :content_type
 attributes :docker_upstream_name
 attributes :unprotected, :full_path, :checksum_type, :container_repository_name
+if (@object && @object.yum?) || (@resource.is_a?(Katello::Repository) && @resource.yum?)


@ehelms @cfouant had to fix this as @resource could be defined as something other than Repository (e.g. ContentViewFilter). Also, @object isn't defined when the controller renders this directly. Are you cool with this?

I would prefer we just show download policy on all repos. @ehelms your thoughts?

That's fine.

cfouant · 2016-02-04T18:41:09Z

I'd ACK this except I wrote a bunch of it myself. I have tested it a bunch and seems to work for me 👍

ehelms · 2016-02-05T01:45:24Z

Looks like this is going to need a rebase after the cassette regeneration.

ehelms · 2016-02-05T02:29:19Z

db/migrate/20160131182301_add_download_policy_to_katello_repositories.rb

+  end
+  def change
+    add_column :katello_repositories, :download_policy, :string
+    DownloadPolicyRepository.where(content_type: 'yum').update_all(download_policy: 'immediate')


Does this need an upgrade task to change all existing repositories in Pulp

No. Pulp defaults to immediate.

ehelms · 2016-02-05T02:34:49Z

I tried to create a repository and ran into:

2016-01-31T07:22:24 [foreman-tasks/action] [E] Validation failed: Download policy is not included in the list (ActiveRecord::RecordInvalid)
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/validations.rb:57:in `save!'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/attribute_methods/dirty.rb:29:in `save!'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/transactions.rb:273:in `block in save!'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/transactions.rb:329:in `block in with_transaction_returning_status'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/connection_adapters/abstract/database_statements.rb:199:in `transaction'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/transactions.rb:208:in `transaction'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/transactions.rb:326:in `with_transaction_returning_status'
 | /home/vagrant/.rvm/gems/ruby-2.2.4/gems/activerecord-4.1.5/lib/active_record/transactions.rb:273:in `save!'
 | /home/vagrant/katello/app/lib/katello/lazy_accessor.rb:87:in `save!'
 | /home/vagrant/katello/app/lib/actions/katello/repository/create.rb:7:in `plan'

daviddavis · 2016-02-05T14:46:07Z

Waiting on #5754

ehelms · 2016-02-05T14:51:44Z

I am a bit confused -- if I need all of these PRs for repository creation to work, shouldn't they be combined together?

daviddavis · 2016-02-05T14:55:46Z

The scope of this PR is to get the API working. If we want everything to work (API, UI, etc) it seems like we might as well just create a feature branch?

ehelms · 2016-02-05T14:59:30Z

I don't think we need a feature branch, and best to avoid those where possible. This is how I look at it, if a single PR goes in and breaks basic functionality that's bad. Further, if I need to cherry pick I might end up in a broken state. So I am all in favor of adding in lazy sync changes but where it keeps the lights on.

daviddavis · 2016-02-05T15:03:27Z

It sounds easy in theory but having to address comments across a list of commits is a huge pain. If we get feedback or we need to change something, we either end up with new commits or we have to somehow rebase it into the old commit. Dealing with one or two commits per PR is much easier. Plus we're also dealing with being dependent on the nodes work which we could just merge into our branch even if it's not done. I agree with your idea in theory but in practice it's not as easy.

ehelms · 2016-02-05T15:14:53Z

Let me backup, as you are right I should look at this from the API perspective. Which, fails with the same error as the UI. You argue I need to include download_policy as an attribute in my API call, but the docs do not indicate that it is a required field. Which is why I argue if #5752 accounts for that and fixes the UI that they should all be a single commit.

daviddavis · 2016-02-05T15:17:39Z

Ok I'll add it in. You will need to test from the API unless you check out this commit locally:

cfouant@fb85b41

ehelms · 2016-02-05T15:54:30Z

app/models/katello/glue/pulp/repos.rb

        unprotected = unprotected.nil? ? false : unprotected
+        download_policy = Setting[:default_download_policy] if download_policy.blank? && yum?


self in this context is a product not a repository thus yum? fails here with no method error. I think you'll need to check the repo_type parameter passed in.

daviddavis · 2016-02-05T19:38:23Z

ACK

ehelms · 2016-02-05T19:48:20Z

LOL this is your own PR.... however

ACK

Fixes #13485 - API: Allow user to set sync type per repository

theforeman-bot added Needs testing Not yet reviewed labels Feb 1, 2016

cfouant reviewed Feb 1, 2016
View reviewed changes

daviddavis changed the title ~~[Work in Progress] Fixes #13485 - Allows user to set sync type per repository~~ [Work in Progress] Fixes #13485 - API: Allow user to set sync type per repository Feb 1, 2016

ehelms mentioned this pull request Feb 1, 2016

Show download_policy in rabl only for yum repos #5751

Closed

jlsherrill reviewed Feb 2, 2016
View reviewed changes

theforeman-bot added Waiting on contributor Needs testing and removed Waiting on contributor labels Feb 2, 2016

daviddavis changed the title ~~[Work in Progress] Fixes #13485 - API: Allow user to set sync type per repository~~ Fixes #13485 - API: Allow user to set sync type per repository Feb 2, 2016

cfouant mentioned this pull request Feb 2, 2016

Fixes #13485 - Shows download policy on repository Katello/hammer-cli-katello#344

Merged

daviddavis mentioned this pull request Feb 2, 2016

Fixes #13474 - Allow users to set a default setting for lazy sync #5752

Closed

theforeman-bot added Waiting on contributor Needs testing and removed Waiting on contributor labels Feb 3, 2016

ehelms reviewed Feb 3, 2016
View reviewed changes

ehelms added Waiting on contributor and removed Not yet reviewed labels Feb 3, 2016

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Feb 3, 2016

daviddavis reviewed Feb 3, 2016
View reviewed changes

ehelms reviewed Feb 5, 2016
View reviewed changes

daviddavis added the Blocked by another PR label Feb 5, 2016

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Feb 5, 2016

ehelms reviewed Feb 5, 2016
View reviewed changes

daviddavis added Waiting on contributor and removed Needs re-review Needs testing Blocked by another PR labels Feb 5, 2016

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Feb 5, 2016

daviddavis added Waiting on contributor and removed Needs re-review Needs testing labels Feb 5, 2016

Fixes #13485,#13474 - API: Allows user to set sync type per repository

d97678a

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Feb 5, 2016

daviddavis added a commit that referenced this pull request Feb 5, 2016

Merge pull request #5749 from daviddavis/lazy-sync-api

856abe8

Fixes #13485 - API: Allow user to set sync type per repository

daviddavis merged commit 856abe8 into Katello:master Feb 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #13485 - API: Allow user to set sync type per repository #5749

Fixes #13485 - API: Allow user to set sync type per repository #5749

daviddavis commented Feb 1, 2016

cfouant Feb 1, 2016

parthaa commented Feb 1, 2016

cfouant commented Feb 1, 2016

jlsherrill Feb 2, 2016

cfouant Feb 2, 2016

ehelms Feb 3, 2016

daviddavis Feb 3, 2016

ehelms Feb 3, 2016

daviddavis Feb 3, 2016

cfouant Feb 3, 2016

ehelms Feb 3, 2016

cfouant commented Feb 4, 2016

ehelms commented Feb 5, 2016

ehelms Feb 5, 2016

daviddavis Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

		unprotected = unprotected.nil? ? false : unprotected
		download_policy = Setting[:default_download_policy] if download_policy.blank? && yum?

Fixes #13485 - API: Allow user to set sync type per repository #5749

Fixes #13485 - API: Allow user to set sync type per repository #5749

Conversation

daviddavis commented Feb 1, 2016

Choose a reason for hiding this comment

parthaa commented Feb 1, 2016

cfouant commented Feb 1, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfouant commented Feb 4, 2016

ehelms commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016

daviddavis commented Feb 5, 2016

Choose a reason for hiding this comment

daviddavis commented Feb 5, 2016

ehelms commented Feb 5, 2016