Fixes #24569 - Expose minor/major cvv version in API. #7594

chris1984 · 2018-08-02T17:23:14Z

No description provided.

ehelms · 2018-08-06T13:48:20Z

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

@@ -79,9 +79,13 @@ def update
    param :description, String, :desc => N_("Description for the new published content view version")
    param :force_yum_metadata_regeneration, :bool, :desc => N_("Force yum metadata regeneration on the repositories " \
                                                           "in the content view version")
+    param :major, :number, :desc => N_("Override the major version number provided by Katello"), :required => false
+    param :minor, :number, :desc => N_("Override the minor version number provided by Katello"), :required => false


I think we could drop the provided by in the description

ehelms · 2018-08-06T13:51:40Z

app/lib/actions/katello/content_view/publish.rb

@@ -9,7 +9,11 @@ class Publish < Actions::EntryAction
        def plan(content_view, description = "", options = {})
          action_subject(content_view)
          content_view.check_ready_to_publish!
-          version = content_view.create_new_version
+          if options[:minor] && options[:major]


I think we may need to add some validation at the model level to ensure these are both integer values now that we are allowing customization of them. The major field is an integer in the database but minor is defined as a string which would allow anything. I checked buy didn't see any explicit validations for these fields. Can double check with @beav or @jlsherrill in case I missed them.

We probably do need to check that they are numeric and that no existing CVV exists with those versions. In fact it doesn't even look like there is a unique index on those versions, so that needs to be added to.

👍 to numeric check, I was able to set "five" as the minor string and it resulted in a successful task.

@beav Added a check to the model to make sure :minor is an integer.

jlsherrill · 2018-08-08T13:47:56Z

db/migrate/20180806205420_add_index_to_content_view_version.rb

+class AddIndexToContentViewVersion < ActiveRecord::Migration[5.1]
+  def change
+    add_index :katello_content_view_versions, :major
+    add_index :katello_content_view_versions, :minor


For this index, we need it to be unique across 3 columns, content_view_id, major, and minor. It'd look something like this:

add_index :katello_content_view_versions, [:content_view_id, :major, :minor], :unique => true

I think we want to add a uniquness validation to the model as well, something like:

validates :minor :uniqueness => {:scope => [:content_view_id, :major]}

jlsherrill · 2018-08-13T14:49:08Z

@chris1984 looks like there is still a test failure

beav · 2018-08-14T16:38:16Z

app/models/katello/content_view_version.rb

@@ -44,6 +44,8 @@ class ContentViewVersion < Katello::Model

    validates_lengths_from_database

+    validates :minor, :uniqueness => {:scope => [:content_view_id, :major], :message => N_("Major, and content_view_id trio must be unique")}


This message auto-assumes that it will start with the word "minor" since that's the key its validating. The message should be something like ", major, and content_view_id trio must be unique" for it to print properly

As in they must all three be unique or unique within the scope of each other? Wondering if we can find a word or two that encompasses that for clarity. Trio makes me think I am ordering an appetizer, which makes me hungry.

this turned out to be a fixture issue, we are fixing now.

here's an appetizer trio though 🍣 🥘 🍗

beav · 2018-08-14T19:16:11Z

app/lib/actions/katello/environment/library_create.rb

-          end
-
-          version = library_view.versions.first
+          version = library_view.versions.find_by(:major => 1)


@jlsherrill @ehelms we weren't sure on this one. In tests, it looks like CVV version 1.0 of the DOV is already created, which makes this create! fail. We aren't sure if the CVVs on the DOV are actually used anywhere or not.

I feel a bit dumb asking this, what does DOV stand for?

default organization view, it is the content view that is just Library

beav · 2018-08-15T15:11:56Z

app/lib/actions/katello/content_view/publish.rb

-          version = content_view.create_new_version
+          if options[:minor] && options[:major]
+            version = content_view.create_new_version(options[:major], options[:minor])
+            increment!(:next_version)


is this needed? I get

2018-08-15T15:08:14 [E|bac|926a6] undefined method `increment!' for #<Actions::Katello::ContentView::Publish:0x00007f2a6ea06c58> (NoMethodError) /opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.7.0/app/lib/actions/katello/content_view/publish.rb:14:in `plan' /opt/theforeman/tfm/root/usr/share/gems/gems/dynflow-1.0.5/lib/dynflow/action.rb:493:in `block (3 levels) in execute_plan' /opt/theforeman/tfm/root/usr/share/gems/gems/dynflow-1.0.5/lib/dynflow/middleware/stack.rb:26:in `pass' /opt/theforeman/tfm/root/usr/share/gems/gems/dynflow-1.0.5/lib/dynflow/middleware.rb:18:in `pass'

when calling the api like this:

http --verify=no -a admin:changeme -b POST https://hostname/katello/api/v2/content_views/3/publish major=10 minor=5

chris1984 · 2018-08-16T20:10:27Z

@beav @ehelms updated with all concerns. Tests are green as well, can we get a merge?

beav · 2018-08-16T22:44:58Z

@chris1984 looks good, will retest tomorrow

beav · 2018-08-17T14:58:15Z

The following sequence failed:

publish CV version 1.0 (do not set major/minor)
publish CV 3.0 (setting major/minor)
publish CV version 2.0 (do not set, rely on autoincrement)
publish CV again without setting major/minor

It's OK that this failed, but there's no way to make publish work again without setting the major/minor. It looks like the autoincrement needs to bump to whatever major+1 is after publishing with major/minor set

beav · 2018-08-20T21:04:12Z

app/models/katello/content_view.rb

@@ -494,7 +494,7 @@ def create_new_version(major = next_version, minor = 0, components = self.compon
                                           :components => components
                                          )

-      increment!(:next_version) if minor == 0
+      next_version = major + 1


I'm not able to publish after this change:

no implicit conversion of Integer into String (TypeError) | /home/vagrant/katello/app/models/katello/content_view.rb:497:in `+' | /home/vagrant/katello/app/models/katello/content_view.rb:497:in `create_new_version' | /home/vagrant/katello/app/lib/actions/katello/content_view/publish.rb:13:in `plan'

this seems to help:

self.next_version = major.to_i + 1

However I'm not sure how to get the change to persist. The old increment! method would auto-save, and I'm not sure where the correct place to do the save is now. We can look at it more tomorrow.

Since the old version saved here, i would just do that here too.

update_attributes(:next_version = major.to_i + 1)

But i think there are some conditions that are missed here:

what if the major version being passed in is older than the current 'next_version'

for incremental update, you wouldn't want to increment the major if only the minor version is being updated (although maybe it doesn't matter if you handle 1)

sean797 · 2018-08-22T10:52:51Z

test/controllers/api/v2/content_views_controller_test.rb

@@ -85,6 +86,14 @@ def test_create
      assert_template 'katello/api/v2/common/create'
    end

+    def test_create_with_version_params
+      post :create, params: { :name => "My View", :label => "My_View", :major => 2, :minor => 1, :description => "Cool", :organization_id => @organization.id }


This should be publish not create, can you also verify that the version with those major and minor versions exist after?

here's a similar test

katello/test/controllers/api/v2/content_views_controller_test.rb

Lines 255 to 261 in 0095469

def test_publish_default_view

view = ContentView.find(katello_content_views(:acme_default).id)

version_count = view.versions.count

post :publish, params: { :id => view.id }

assert_response 400

assert_equal version_count, view.versions.reload.count

end

Bonus points if you can make use of @publish_permission ;)

I think the right approach for a controller test which goes to one or more backend services is simply to test that the action is invoked properly. Testing that the major & minor versions exist is the responsibility of a different test (in my opinion!).

chris1984 · 2018-08-29T17:43:24Z

@beav @sean797 updated.

beav · 2018-08-29T17:53:37Z

🇬🇱 🍏 🥗

beav · 2018-08-29T17:57:30Z

The changes look good to me, but I worked with a preliminary version of this patch a lot and might not be a good reviewer for final ACK since I'm already familiar with it.

Can someone else provide final ack?

jlsherrill · 2018-08-30T22:23:33Z

Thanks @chris1984

Previously, the only way to control which packages land in content view version was by using filters, or by managing the contents of the Library versions of the repos. This commit lets you optionally specify the exact package set you want in your repository. It will look at the Library version of each repo, and copy the list of packages into the CV version. Example value for `repos_units` when calling `/katello/api/v2/content_views/2/publish`: ```json { "repos": [ { "label": "zoo", "rpm_filenames": [ "walrus-5.21-1.noarch.rpm", "gorilla-0.62-1.noarch.rpm" ] }, { "label": "a_longer_label", "rpm_filenames": [ "facter-2.4.6-3.el7sat.x86_64.rpm", "pulp-rpm-handlers-2.13.4.9-1.el7sat.noarch.rpm" ] } ] } ``` This works the same for custom and RH repos. When used with Katello#7594, you can set the CV version number as well as the list of packages in each repo. This is useful for cloning a CV version from one Katello to another.

Previously, the only way to control which packages land in content view version was by using filters, or by managing the contents of the Library versions of the repos. This commit lets you optionally specify the exact package set you want in your repository. It will look at the Library version of each repo, and copy the list of packages into the CV version. Example value for `repos_units` when calling `/katello/api/v2/content_views/2/publish`: ```json [ { "repo_label": "zoo", "rpm_filenames": [ "walrus-5.21-1.noarch.rpm", "gorilla-0.62-1.noarch.rpm" ] }, { "repo_label": "Red_Hat_Satellite_Tools_6_3_for_RHEL_7_Server_RPMs_x86_64", "rpm_filenames": [ "facter-2.4.6-3.el7sat.x86_64.rpm", "pulp-rpm-handlers-2.13.4.9-1.el7sat.noarch.rpm" ] } ] ``` This works the same for custom and RH repos. When used with Katello#7594, you can set the CV version number as well as the list of packages in each repo. This is useful for cloning a CV version from one Katello to another.

Previously, the only way to control which packages land in content view version was by using filters, or by managing the contents of the Library versions of the repos. This commit lets you optionally specify the exact package set you want in your repository. It will look at the Library version of each repo, and copy the list of packages into the CV version. Example value for `repos_units` when calling `/katello/api/v2/content_views/2/publish`: ```json [ { "repo_label": "zoo", "rpm_filenames": [ "walrus-5.21-1.noarch.rpm", "gorilla-0.62-1.noarch.rpm" ] }, { "repo_label": "a_longer_label", "rpm_filenames": [ "facter-2.4.6-3.el7sat.x86_64.rpm", "pulp-rpm-handlers-2.13.4.9-1.el7sat.noarch.rpm" ] } ] ``` This works the same for custom and RH repos. When used with Katello#7594, you can set the CV version number as well as the list of packages in each repo. This is useful for cloning a CV version from one Katello to another.

Previously, the only way to control which packages land in content view version was by using filters, or by managing the contents of the Library versions of the repos. This commit lets you optionally specify the exact package set you want in your repository. It will look at the Library version of each repo, and copy the list of packages into the CV version. Example value for `repos_units` when calling `/katello/api/v2/content_views/2/publish`: ```json [ { "label": "zoo", "rpm_filenames": [ "walrus-5.21-1.noarch.rpm", "gorilla-0.62-1.noarch.rpm" ] }, { "label": "a_longer_label", "rpm_filenames": [ "facter-2.4.6-3.el7sat.x86_64.rpm", "pulp-rpm-handlers-2.13.4.9-1.el7sat.noarch.rpm" ] } ] ``` This works the same for custom and RH repos. When used with Katello#7594, you can set the CV version number as well as the list of packages in each repo. This is useful for cloning a CV version from one Katello to another.

Previously, the only way to control which packages land in content view version was by using filters, or by managing the contents of the Library versions of the repos. This commit lets you optionally specify the exact package set you want in your repository. It will look at the Library version of each repo, and copy the list of packages into the CV version. Example value for `repos_units` when calling `/katello/api/v2/content_views/2/publish`: ```json [ { "label": "zoo", "rpm_filenames": [ "walrus-5.21-1.noarch.rpm", "gorilla-0.62-1.noarch.rpm" ] }, { "label": "a_longer_label", "rpm_filenames": [ "facter-2.4.6-3.el7sat.x86_64.rpm", "pulp-rpm-handlers-2.13.4.9-1.el7sat.noarch.rpm" ] } ] ``` This works the same for custom and RH repos. When used with #7594, you can set the CV version number as well as the list of packages in each repo. This is useful for cloning a CV version from one Katello to another.

theforeman-bot added Needs testing Not yet reviewed labels Aug 2, 2018

ehelms reviewed Aug 6, 2018

View reviewed changes

jlsherrill reviewed Aug 8, 2018

View reviewed changes

chris1984 changed the title ~~[WIP] Add CV Version API params.~~ Fixes #24569 - Expose minor/major cvv version in API. Aug 8, 2018

chris1984 changed the title ~~Fixes #24569 - Expose minor/major cvv version in API.~~ Fixes #24569 - Expose minor/major cvv version in API Aug 8, 2018

chris1984 mentioned this pull request Aug 8, 2018

Add source url param to Katello module theforeman/foreman-ansible-modules#163

Closed

chris1984 changed the title ~~Fixes #24569 - Expose minor/major cvv version in API~~ Fixes #24569 - Expose minor/major cvv version in API. Aug 14, 2018

beav reviewed Aug 14, 2018

View reviewed changes

beav reviewed Aug 15, 2018

View reviewed changes

beav reviewed Aug 20, 2018

View reviewed changes

sean797 reviewed Aug 22, 2018

View reviewed changes

Fixes #24569 - Expose minor/major cvv version in API.

8fe4090

jlsherrill approved these changes Aug 30, 2018

View reviewed changes

jlsherrill merged commit d95e9b7 into Katello:master Aug 30, 2018

beav mentioned this pull request Sep 11, 2018

Fixes #24902 - pass in list of packages when publishing CV #7696

Merged

chris1984 mentioned this pull request Sep 13, 2018

Refs #24569 - Add 3.9 apidata & tests for cv publish version. Katello/hammer-cli-katello#587

Merged

akofink mentioned this pull request Sep 18, 2018

Fixes #24937: Never CVV auto_publish when not comp #7703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #24569 - Expose minor/major cvv version in API. #7594

Fixes #24569 - Expose minor/major cvv version in API. #7594

chris1984 commented Aug 2, 2018

ehelms Aug 6, 2018

ehelms Aug 6, 2018

jlsherrill Aug 6, 2018

beav Aug 17, 2018

chris1984 Aug 20, 2018

jlsherrill Aug 8, 2018

jlsherrill Aug 8, 2018

jlsherrill commented Aug 13, 2018

beav Aug 14, 2018

ehelms Aug 15, 2018

beav Aug 15, 2018

chris1984 Aug 16, 2018

beav Aug 14, 2018

ehelms Aug 15, 2018

beav Aug 15, 2018

beav Aug 15, 2018 •

edited

chris1984 commented Aug 16, 2018

beav commented Aug 16, 2018

beav commented Aug 17, 2018 •

edited

beav Aug 20, 2018

beav Aug 20, 2018

jlsherrill Aug 21, 2018

sean797 Aug 22, 2018 •

edited

jturel Aug 29, 2018

chris1984 commented Aug 29, 2018

beav commented Aug 29, 2018

beav commented Aug 29, 2018

jlsherrill commented Aug 30, 2018

		@@ -44,6 +44,8 @@ class ContentViewVersion < Katello::Model

		validates_lengths_from_database

		validates :minor, :uniqueness => {:scope => [:content_view_id, :major], :message => N_("Major, and content_view_id trio must be unique")}

	def test_publish_default_view
	view = ContentView.find(katello_content_views(:acme_default).id)
	version_count = view.versions.count
	post :publish, params: { :id => view.id }
	assert_response 400
	assert_equal version_count, view.versions.reload.count
	end

Fixes #24569 - Expose minor/major cvv version in API. #7594

Fixes #24569 - Expose minor/major cvv version in API. #7594

Conversation

chris1984 commented Aug 2, 2018

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

jlsherrill commented Aug 13, 2018

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

beav Aug 15, 2018 • edited

Choose a reason for hiding this comment

chris1984 commented Aug 16, 2018

beav commented Aug 16, 2018

beav commented Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean797 Aug 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 commented Aug 29, 2018

beav commented Aug 29, 2018

beav commented Aug 29, 2018

jlsherrill commented Aug 30, 2018

beav Aug 15, 2018 •

edited

beav commented Aug 17, 2018 •

edited

sean797 Aug 22, 2018 •

edited