-
Notifications
You must be signed in to change notification settings - Fork 31
Fixes for Satellite 6.3. This include PR #50, #43 #53
base: master
Are you sure you want to change the base?
Conversation
…tellite#43 This fix override also PR RedHatSatellite#52, making it obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall NACK, mostly because it's too many changes in one commit :)
README.md
Outdated
@@ -77,7 +91,8 @@ Example configuration for `cvmanager`: | |||
- application1 | |||
|
|||
* `user`: username of a Satellite 6 user to execute the actions with | |||
* `pass`: password of the same user | |||
* `pass`: password of the same user in cleartext | |||
* `encoded_pass`: password of the same user in base64 encryption (generate with 'echo -n "sat_password" | base64') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK, base64 is not an encryption and we should not pretend it to be one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is encoded fine as description or do you prefer something like "masquerade_pass"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I prefer not to have this "feature" ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
barely_obfuscated_pass ?
Willing to discuss every single change :) |
I merged the labels PR, please rebase, drop the base64 changes and we'll re-discuss. |
Uploaded as requested without encoded_pass option. |
@@ -198,14 +222,14 @@ def clean() | |||
end | |||
|
|||
def checktask(task, last_date) | |||
task_completed_at = Time.xmlschema(task['ended_at']) rescue Time.parse(task['ended_at']) | |||
task_completed_at = Time.xmlschema(task['started_at']) rescue Time.parse(task['started_at']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? If we use started_at
, we might find tasks that were started before last_date
, but did not complete, and thus were not part of the last publish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bandrea83 I do not recall the details. May you please check?
As reference: f3b7a3a
# Check every repo in the CV | ||
cv['repository_ids'].each do |repo_id| | ||
# get repo data | ||
repo = @api.resource(:repositories).call(:show, {:id => repo_id}) | ||
# check if the last sync has been ever completed | ||
if repo.has_key?('last_sync') and repo['last_sync'] and repo['last_sync'].has_key?('ended_at') and repo['last_sync']['ended_at'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above, why changing to started_at
I tested it this morning after everyone made changes and committed the pull request, where they changed
the YAML is
the reason I changed it to I forced a publish and got
so I can try update again. |
this works here:
|
Can you provide your |
@@ -334,10 +397,18 @@ def promote() | |||
if not @options[:noop] | |||
req = @api.resource(:content_view_versions).call(:promote, {:id => latest_version['id'], :environment_id => @options[:lifecycle], :force => @options[:force]}) | |||
tasks << req['id'] | |||
wait([req['id']]) if @options[:sequential] | |||
#if @options[:sequential] > 0 and tasks.length >= @options[:sequential] # Why are not we using this standard code? | |||
if @options[:sequential] > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evgeni do you know / recall why here a different code is used?
if @options[:sequential] > 0
in place of if @options[:sequential] > 0 and tasks.length >= @options[:sequential]
@gearboxscott thanks for taking the time to test it. |
This fix override also PR #52, making it obsolete.
Also, it include some improvement in clean section.
CCV were possibly done before CV, resulting in a error.
Please note that bug http://projects.theforeman.org/issues/23458 may occur.