merge changes for 6.3 in master. #52

gearboxscott · 2018-04-27T20:26:58Z

make the changes necessary for cvmanager to work with Satellite version 6.3.

…g composite views with the latest versions of content views.

mccun934 · 2018-04-27T20:39:25Z

@evgeni talked to Scott about getting master updated to 6.3 support.

We should probably branch master as it is now to a new satellite-6.2 branch so users can clone depending on the version they want. I'll let you review the changes since I'm not familiar with this code.

Rocco83

I can be blind, but the code is duplicated from row 25 on.

Rocco83 · 2018-04-27T22:59:59Z

@mccun934 @evgeni I personally think that we need to not let go PR before moving to 6.3.
My very brief review is above. The code seems to me duplicated in the main file.
@gearboxscott am i correct? I have deleted from 518 on, diff on the current master, and seems to me at least a proper diff.
You have kept a couple of # end loop which i think are in @evgeni style. But up to him.

I would suggest a review of the PR anyway.

gearboxscott · 2018-04-27T23:09:01Z

@mccun934 @Rocco83

some how when I uploaded from my copy, it duplicated. Let me re-upload it again and fix the issue.

Scott

Rocco83 · 2018-04-27T23:12:39Z

cvmanager

@@ -54,7 +47,7 @@ require 'time'
 optparse = OptionParser.new do |opts|
  opts.banner = "Usage: #{opts.program_name} ACTION [options]"
  opts.version = "0.1"
-
+  


Useless white space

gearboxscott · 2018-04-27T23:13:05Z

@mccun934 @Rocco83

I uploaded a new copy of the code to my branch, can you see it?

gearboxscott · 2018-04-27T23:14:56Z

@mccun934 @Rocco83

I didn't realize that it did that because I didn't look at it when I uploaded it to github.com. Should be better now.

Scott

Rocco83 · 2018-04-27T23:16:45Z

cvmanager

    end

+    was_updated = true


Why always was_updated = true?

The following is taken as example from a CCV API call

content_view_components 0 latest false <-- this is true if latest is selected.

Given that, if latest exists and is true, then we can proceed.
Also, I think that the best would be to check anyway if we have the latest version or not of a CV in a CCV.
The reason is, we are going to publish anyway always a new CCV for nothing.

The whole idea of cvmanager is to do actions only if necessary, i think.

My 2 cents,

Yeah, latest should be a special case, and given the API tells us about it, we can handle it properly, instead of removing the code that handles explicit versions.

That's the only 6.3 change, right?

Rocco83 · 2018-04-27T23:17:11Z

cvmanager

-        # do the update
-        was_updated = true
-      end
+    # end loop


Stylistic approach, i think to be agreed with @evgeni

Rocco83 · 2018-04-30T14:29:27Z

README.md

+gem install apipie
+```
+
+This version will no longer work with Satellite 6.2 or less.


Why not to make an API call to check if latest is supported or not?
So the code may be backward compatible.

@Rocco83, when @mccun934 told me to merge the changes into the current code for 6.3 and they can download a prior release for 6.2 or less. We can add this functionality but then we need to add in the update section of the code a branch for 6.2 or less. @mccun934 do you me to do that or have them download a prior release?

How big is the diff between 6.2 and 6.3 compat? I'd prefer to remain 6.2 compatibility, but if it's too much of a burden, I'd tag a last 6.2 compatible release and carry on with the modern code.

Rocco83 · 2018-04-30T14:30:06Z

cvmanager

@@ -250,11 +250,11 @@ def update()
      # either the version for the component in this CCV is set
      # or it is set globally
      # never touch non-mentioned components
-      if @yaml[:ccv].is_a?(Hash) and @yaml[:ccv].has_key?(ccv['label']) and @yaml[:ccv][ccv['label']].has_key?(component['content_view']['label'])
-        desired_version = @yaml[:ccv][ccv['label']][component['content_view']['label']]
+      if @yaml[:ccv].is_a?(Hash) and @yaml[:ccv].has_key?(ccv['label']) and @yaml[:ccv][ccv['label']].has_key?(component['content_view']['name'])


Why do we need to rely on name?
This is against #50

@Rocco83 , keep in mind I'm not the originator of the code, my sole purpose of updated was to fix it for 6.3 so my current customer using the code didn't have their periodic based update process for server calling this from cron breaking. I don't agree with the all the logic here because I would like had a method of chaining a publish+update+promote be in one step. I just wanted get it working for 6.3 customers. If we want to go back a visit the logic and rewrite a good portion of then let do that in a different pull request. Or have completely missed the point of fixing this and we need to rewrite some of the flow?

I do not understand the point, why is a change needed here?
Sorry but i am not getting that.

@Rocco83 when you run the existing code as it was on a 6.3, the composite content view is marked latest, meaning as you know, you don't have to visit the composite view and update the version of the content views within it. You just have to push. The code, was looking for, something to update and didn't know how to recognize that it's already set to latest and it just needs to be published. the update just run and spews out the version but it doesn't publish, it exits. Try it out, run it on a sat 6.2 and then again on 6.3. 6.2 will change, 6.3 will just exit.

I am just referring to this portion of code. Why asking for name instead of label would do the trick?
Am I wrong if i say that what would do the job is in line 274?

Yeah, labels should be labels. IMHO. Need to re-run that code with latest katello, tho.

changed ruby to tfm-ruby

evgeni · 2018-05-02T07:06:46Z

cvmanager

@@ -1,4 +1,4 @@
-#!/usr/bin/env ruby
+#!/usr/bin/env tfm-ruby


I must admit I'd prefer to keep the tfm SCL away from the shebang. There are 1000 ways to get the script working, and tfm-ruby is just one of them.

For example, this also works (and that's how I run cvmanager on our Jenkins):

bundle install bundle exec ruby ./cvmanager …

evgeni · 2018-05-02T07:16:20Z

README.md

@@ -1,3 +1,17 @@
+# Satellite 6.3 Update
+
+Satellite 6.3 no longer provide apipie via the repositories.  To use this utility, install apipie.


Not completely true, we just don't provide apipie-bindings for the system ruby.

I think we should document the various ways users can run it, and let them decide:

the most Ruby way would be bundle:

bundle install bundle exec ruby ./cvmanager …

scl enable tfm -- ./cvmanager works on any production install of Katello/Satellite

gem install apipie-bindings followed by a ./cvmanager also works, but is the most unclean one, imho

evgeni · 2018-05-02T07:17:37Z

README.md

+gem install apipie
+```
+
+This version will no longer work with Satellite 6.2 or less.


How big is the diff between 6.2 and 6.3 compat? I'd prefer to remain 6.2 compatibility, but if it's too much of a burden, I'd tag a last 6.2 compatible release and carry on with the modern code.

evgeni · 2018-05-02T07:17:56Z

README.md

+
+# Origin
+
+This was forked from katello-cvmanger https://github.com/RedHatSatellite/katello-cvmanager and updated to work with Satellite 6.3.


This is not a fork, it's a tribute, erm, PR ;)

evgeni · 2018-05-02T07:22:10Z

cvmanager

    end

+    was_updated = true


Yeah, latest should be a special case, and given the API tells us about it, we can handle it properly, instead of removing the code that handles explicit versions.

That's the only 6.3 change, right?

evgeni · 2018-05-02T07:23:12Z

cvmanager

@@ -250,11 +250,11 @@ def update()
      # either the version for the component in this CCV is set
      # or it is set globally
      # never touch non-mentioned components
-      if @yaml[:ccv].is_a?(Hash) and @yaml[:ccv].has_key?(ccv['label']) and @yaml[:ccv][ccv['label']].has_key?(component['content_view']['label'])
-        desired_version = @yaml[:ccv][ccv['label']][component['content_view']['label']]
+      if @yaml[:ccv].is_a?(Hash) and @yaml[:ccv].has_key?(ccv['label']) and @yaml[:ccv][ccv['label']].has_key?(component['content_view']['name'])


Yeah, labels should be labels. IMHO. Need to re-run that code with latest katello, tho.

removed the use of tfm-ruby

…tellite#43 This fix override also PR RedHatSatellite#52, making it obsolete.

Rocco83 · 2018-05-03T12:57:53Z

I have pushed #53 as alternative. @gearboxscott feel free to post your comment!
Code is anyway not heavily reviewed. A test would be more then appreciated.

gearboxscott added 4 commits April 27, 2018 10:29

Updated to work with Satellite 6.3 which is now automatically updatin…

06812f6

…g composite views with the latest versions of content views.

added comments about using cvmanager with Satellite 6.3.

e298e14

add comment about this version will not work with 6.2 or less.

375c7a4

add origin to show where the original code came from.

54d2a87

mccun934 assigned evgeni Apr 27, 2018

Rocco83 reviewed Apr 27, 2018

View reviewed changes

re-uploaded the file

361bd77

Rocco83 reviewed Apr 27, 2018

View reviewed changes

Rocco83 reviewed Apr 28, 2018

View reviewed changes

Rocco83 reviewed Apr 30, 2018

View reviewed changes

changed ruby to tfm-ruby

7572c36

changed ruby to tfm-ruby

evgeni suggested changes May 2, 2018

View reviewed changes

removed tfm from ruby

7576a29

removed the use of tfm-ruby

Rocco83 added a commit to Rocco83/katello-cvmanager that referenced this pull request May 3, 2018

Fixed for Satellite 6.3. This include PR RedHatSatellite#50, RedHatSa…

c6fb789

…tellite#43 This fix override also PR RedHatSatellite#52, making it obsolete.

Rocco83 mentioned this pull request May 3, 2018

Fixes for Satellite 6.3. This include PR #50, #43 #53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge changes for 6.3 in master. #52

merge changes for 6.3 in master. #52

gearboxscott commented Apr 27, 2018

mccun934 commented Apr 27, 2018

Rocco83 left a comment

Rocco83 commented Apr 27, 2018

gearboxscott commented Apr 27, 2018

Rocco83 Apr 27, 2018

gearboxscott commented Apr 27, 2018

gearboxscott commented Apr 27, 2018

Rocco83 Apr 27, 2018

Rocco83 Apr 30, 2018

evgeni May 2, 2018

Rocco83 Apr 27, 2018

Rocco83 Apr 30, 2018

gearboxscott Apr 30, 2018

evgeni May 2, 2018

Rocco83 Apr 30, 2018

gearboxscott Apr 30, 2018

Rocco83 Apr 30, 2018

gearboxscott May 1, 2018

Rocco83 May 1, 2018

evgeni May 2, 2018

evgeni May 2, 2018

evgeni May 2, 2018

evgeni May 2, 2018

evgeni May 2, 2018

evgeni May 2, 2018

evgeni May 2, 2018

Rocco83 commented May 3, 2018

		@@ -1,3 +1,17 @@
		# Satellite 6.3 Update

		Satellite 6.3 no longer provide apipie via the repositories. To use this utility, install apipie.


		# Origin

		This was forked from katello-cvmanger https://github.com/RedHatSatellite/katello-cvmanager and updated to work with Satellite 6.3.

merge changes for 6.3 in master. #52

Are you sure you want to change the base?

merge changes for 6.3 in master. #52

Conversation

gearboxscott commented Apr 27, 2018

mccun934 commented Apr 27, 2018

Rocco83 left a comment

Choose a reason for hiding this comment

Rocco83 commented Apr 27, 2018

gearboxscott commented Apr 27, 2018

Choose a reason for hiding this comment

gearboxscott commented Apr 27, 2018

gearboxscott commented Apr 27, 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

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

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

Rocco83 commented May 3, 2018