Fixes #19179 - Add force full sync to proxy syncs #6869
Conversation
|
@johnpmitsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jlsherrill, @iNecas and @tstrachota to be potential reviewers. |
|
Issues: #19179 |
| task = async_task(::Actions::Katello::CapsuleContent::Sync, capsule_content, :environment => @environment) | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, | ||
| capsule_content, | ||
| { :environment => @environment, :skip_metadata_check => params[:capsule_content][:skip_metadata_check] }) |
There was a problem hiding this comment.
Redundant curly braces around a hash parameter.
| def sync | ||
| find_environment if params[:environment_id] | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, capsule_content, :environment => @environment) | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, | ||
| capsule_content, |
| def sync | ||
| find_environment if params[:environment_id] | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, capsule_content, :environment => @environment) | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, |
| Force full sync will sync all repositories' | translate }}" | ||
| tooltip-animation="false" | ||
| tooltip-append-to-body="true"> | ||
| </i> |
There was a problem hiding this comment.
@waldenraines Any ideas on how to get these to be inline?
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
c4ec61d
to
e5e5f60
Compare
| if (!$scope.syncState.is(syncState.SYNCING)) { | ||
|
|
||
| $scope.syncErrorMessages = []; | ||
| $scope.syncState.set(syncState.SYNC_TRIGGERED); | ||
|
|
||
| CapsuleContent.sync({id: capsuleId}).$promise.then(function (task) { | ||
| CapsuleContent.sync({id: capsuleId, skip_metadata_check: skipMetadataCheck}).$promise.then(function (task) { |
There was a problem hiding this comment.
@waldenraines this works, but I am not sure if am going about things the right way here, can you take a look?
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
e5e5f60
to
e61b4d7
Compare
| task = async_task(::Actions::Katello::CapsuleContent::Sync, capsule_content, :environment => @environment) | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, | ||
| capsule_content, | ||
| :environment => @environment, :skip_metadata_check => params[:capsule_content][:skip_metadata_check]) |
There was a problem hiding this comment.
@jlsherrill should I be using [:capsule_content] with these params? if I do params[:skip_metadata_check] the boolean is returned as a string. What is best practices here?
From: /home/vagrant/katello/app/controllers/katello/api/v2/capsule_content_controller.rb @ line 56 Katello::Api::V2::CapsuleContentController#sync:
53: def sync
54: find_environment if params[:environment_id]
55: binding.pry
=> 56: task = async_task(::Actions::Katello::CapsuleContent::Sync,
57: capsule_content,
58: :environment => @environment, :skip_metadata_check => params[:capsule_content][:skip_metadata_check])
59: respond_for_async :resource => task
60: end
[1] pry(#<Katello::Api::V2::CapsuleContentController>)> params
=> {"id"=>"2-almond-proxy-example-com",
"skip_metadata_check"=>true,
"api_version"=>"v2",
"controller"=>"katello/api/v2/capsule_content",
"action"=>"sync",
"capsule_content"=>{"id"=>"2-almond-proxy-example-com", "skip_metadata_check"=>true}}
There was a problem hiding this comment.
Best practice would be to use the foreman boolean helper:
::Foreman::Cast.to_bool(params[:skip_metadata_check]
| @@ -122,13 +122,13 @@ angular.module('Bastion.capsule-content').controller('CapsuleContentController', | |||
|
|
|||
| $scope.isTaskInProgress = isTaskInProgress; | |||
|
|
|||
| $scope.syncCapsule = function () { | |||
| $scope.syncCapsule = function (skipMetadataCheck = false) { | |||
There was a problem hiding this comment.
@waldenraines the linter doesn't seem to like this default argument, is there another way I should be writing this?
Running "eslint:target" (eslint) task
app/assets/javascripts/bastion_katello/capsule-content/capsule-content.controller.js
125:58 error Unexpected token =
✖ 1 problem (1 error, 0 warnings)
There was a problem hiding this comment.
That looks like a JS syntax error.
There was a problem hiding this comment.
I don't think that default functional parameters are valid in es5.
johnpmitsch
force-pushed
the
fpc_force_sync
branch
3 times, most recently
from
1fd3bf0
to
bb535c1
Compare
|
[test] |
| @@ -26,7 +26,6 @@ def plan(repo, pulp_sync_task_id = nil, options = {}) | |||
| pulp_sync_options = {} | |||
| pulp_sync_options[:download_policy] = ::Runcible::Models::YumImporter::DOWNLOAD_ON_DEMAND if validate_contents | |||
| pulp_sync_options[:force_full] = true if skip_metadata_check | |||
| pulp_sync_options[:auto_publish] = false if skip_metadata_check #skip auto publish, and force it later | |||
There was a problem hiding this comment.
Was this removed intentionally? Is this sync action used for proxies or only for the katello server?
There was a problem hiding this comment.
@jlsherrill and I found out that this option actually doesn't do anything in pulp and we decided to remove it in this PR.
There was a problem hiding this comment.
hang one bit with this, i think something is going here, because I basically used this option to reproduce an issue here: #6830 and it seemed to work for me and the reviewer
There was a problem hiding this comment.
nevermind, proceed with this. I chatted with the pulp team and this option won't do anything
| Synchronize | ||
| <span class="caret"></span> | ||
| </button> | ||
| <ul class="dropdown-menu" aria-labelledby="syncDropdown"> |
There was a problem hiding this comment.
Would it also make sense to offer the 'Validate Content Sync' option that is available as an Advanced Sync option for repositories?
There was a problem hiding this comment.
@bbuckingham that one is a bit more difficult to implement so I just added this option for now, I think it would be best to follow up with the validate option in a later PR if we deem it necessary
|
Testing of basic scenarios using the new options, behaved as expected and was consistent with the same options provided for a repo sync (e.g. complete & optimized). |
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
bb535c1
to
655b77a
Compare
| task = async_task(::Actions::Katello::CapsuleContent::Sync, capsule_content.capsule, :environment_id => @environment.try(:id)) | ||
| skip_metadata_check = ::Foreman::Cast.to_bool(params[:skip_metadata_check]) | ||
| task = async_task(::Actions::Katello::CapsuleContent::Sync, | ||
| capsule_content.capsule, |
johnpmitsch
force-pushed
the
fpc_force_sync
branch
3 times, most recently
from
eec01ab
to
1c3da65
Compare
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
1c3da65
to
6ecdd50
Compare
|
For skip_metadata_check on repo syncs, we're doing a metadata generate with force_full as well: https://github.com/Katello/katello/blob/master/app/lib/actions/katello/repository/sync.rb#L46 I think we still need to do that here. |
johnpmitsch
force-pushed
the
fpc_force_sync
branch
2 times, most recently
from
bea627a
to
e67106f
Compare
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, | ||
| repo, capsule_id: | ||
| capsule_content.capsule.id, |
| sync_options: { remove_missing: repo && ["puppet", "yum"].include?(repo.content_type) }) | ||
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, | ||
| repo, capsule_id: |
| plan_action(Pulp::Consumer::SyncCapsule, | ||
| capsule_id: capsule_content.capsule.id, | ||
| repo_pulp_id: repo_id, | ||
| sync_options: { remove_missing: repo && ["puppet", "yum"].include?(repo.content_type) }) | ||
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, |
There was a problem hiding this comment.
Favor a normal if-statement over a modifier clause in a multiline statement.
Trailing whitespace detected.
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, | ||
| repo, capsule_id: | ||
| capsule_content.capsule.id, |
| sync_options: { remove_missing: repo && ["puppet", "yum"].include?(repo.content_type) }) | ||
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, | ||
| repo, capsule_id: |
| plan_action(Pulp::Consumer::SyncCapsule, | ||
| capsule_id: capsule_content.capsule.id, | ||
| repo_pulp_id: repo_id, | ||
| sync_options: { remove_missing: repo && ["puppet", "yum"].include?(repo.content_type) }) | ||
| sync_options: pulp_options) | ||
| plan_action(Katello::Repository::MetadataGenerate, |
There was a problem hiding this comment.
Favor a normal if-statement over a modifier clause in a multiline statement.
Trailing whitespace detected.
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
e67106f
to
4b2f689
Compare
This allows users to force full syncs on foreman proxies with content. This is helpful when the packages on the katello server do not match the packages on the fpc and the user wants to force a sync of the repos
johnpmitsch
force-pushed
the
fpc_force_sync
branch
from
4b2f689
to
3941bb1
Compare
|
ACK |
This allows users to force full syncs on foreman proxies with
content. This is helpful when the packages on the katello
server do not match the packages on the fpc and the user
wants to force a sync of the repos