Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #14491 - add setting to enable/disable lazy sync #5939

Closed
wants to merge 1 commit into from

Conversation

@johnpmitsch
Copy link
Member

commented Apr 5, 2016

This will allow users to turn off lazy sync, which will remove those options from the UI

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2016

two thoughts:

  1. Not sure if we should change the description here to reflect that only "immediate" will be available if enable_lazy_sync setting is false
  2. If a user switches enable_lazy_sync from true to false and the default download policy is "background" that won't be automatically switched to "immediate", trying to figure out how to do that.
@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2016

Not sure if we want to limit anything on the server side based on this setting? i.e. if enable_lazy_sync is false, validate that "immediate" is the download policy of new repos.

@stbenjam

View changes

engines/bastion_katello/lib/bastion_katello/engine.rb Outdated
@@ -33,6 +33,7 @@ class Engine < ::Rails::Engine
),
:config => {
'consumerCertRPM' => consumer_cert_rpm,
'enable_lazy_sync' => Setting[:enable_lazy_sync],

This comment has been minimized.

Copy link
@stbenjam

stbenjam Apr 5, 2016

Contributor

I think this is the source of your test failure, as the engine is loaded before the Settings table exists. You could maybe do:

!Foreman.in_rake? && Setting[:enable_lazy_sync]

Make sure you're not in rake, and then check the setting.

@stbenjam

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

Not sure if we want to limit anything on the server side based on this setting? i.e. if enable_lazy_sync is false, validate that "immediate" is the download policy of new repos.

I think so, otherwise it could be enabled with hammer or the API, right?

@ehelms

View changes

...on_katello/app/assets/javascripts/bastion_katello/repositories/new/views/repository-new.html Outdated
@@ -65,7 +65,8 @@ <h6 translate>

</div>

<div bst-form-group label="{{ 'Download Policy' | translate }}" ng-show="repository.content_type === 'yum'">
<div bst-form-group label="{{ 'Download Policy' | translate }}"
ng-show="repository.content_type === 'yum' && BastionConfig.enable_lazy_sync">

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 6, 2016

Member

BastionConfig is a service, we don't want to access that directly in the view.

@ehelms

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated
@@ -189,7 +189,7 @@ <h4 translate>Basic Information</h4>
<span class="info-label" translate>Download Policy</span>
<span class="info-value"
bst-edit-select="downloadPolicyDisplay(repository.download_policy)"
readonly="denied('edit_products', product)"
readonly="!(BastionConfig.enable_lazy_sync) || denied('edit_products', product)"

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 6, 2016

Member

Same here -- attach this to a variable in the controller so we are not binding this view to the service.

@ehelms

View changes

app/helpers/katello/concerns/settings_helper_extensions.rb Outdated
if Setting[:enable_lazy_sync]
Hash[::Runcible::Models::YumImporter::DOWNLOAD_POLICIES.collect { |p| [p, p] }].to_json
else
Hash[base_dowload_policies.collect { |p| [p, p] }].to_json

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 6, 2016

Member

So many ppppp's, can we use real variable names here for readability?

This comment has been minimized.

Copy link
@daviddavis

daviddavis Apr 6, 2016

Member

Why not just return {"immediate" => "immediate"}.to_json here?

@ehelms

View changes

app/helpers/katello/concerns/settings_helper_extensions.rb Outdated
base_dowload_policies = ["immediate"]

if Setting[:enable_lazy_sync]
Hash[::Runcible::Models::YumImporter::DOWNLOAD_POLICIES.collect { |p| [p, p] }].to_json

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 6, 2016

Member

So many ppppp's, can we use real variable names here for readability?

@daviddavis

View changes

app/models/setting/katello.rb Outdated
@@ -26,7 +26,8 @@ def self.load_defaults
self.set('pulp_client_key', N_("Path for ssl key used for pulp server auth"), "/etc/pki/katello/private/pulp-client.key"),
self.set('pulp_client_cert', N_("Path for ssl cert used for pulp server auth"), "/etc/pki/katello/certs/pulp-client.crt"),
self.set('remote_execution_by_default', N_("If set to true, use the remote execution over katello-agent for remote actions"), false),
self.set('use_pulp_oauth', N_("use oauth authentication for pulp instead of the default cert based authentication"), false)
self.set('use_pulp_oauth', N_("use oauth authentication for pulp instead of the default cert based authentication"), false),
self.set('enable_lazy_sync', N_("enable pulp's lazy sync feature"), true)

This comment has been minimized.

Copy link
@daviddavis

daviddavis Apr 6, 2016

Member

I'm a bit hesitant to use the term lazy sync here since it doesn't appear anywhere else in Katello or pulp. I believe the term pulp uses is "deferred downloads".

This comment has been minimized.

Copy link
@thomasmckay

thomasmckay Apr 6, 2016

Member

Agree with @daviddavis. The UI and hammer refer to this as "download policy" so calling it "enable_download_policies" would be my vote.

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch Apr 6, 2016

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch Apr 6, 2016

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

updated

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

I am still unsure about my 2 comments here if anyone wants to address :)

(to clarify, just 1 and 2 in that specific comment, not the one below)

@stbenjam

View changes

engines/bastion_katello/lib/bastion_katello/engine.rb Outdated
@@ -33,6 +33,7 @@ class Engine < ::Rails::Engine
),
:config => {
'consumerCertRPM' => consumer_cert_rpm,
'enable_download_policies' => !Foreman.in_rake? && Setting[:enable_download_policies],

This comment has been minimized.

Copy link
@stbenjam

stbenjam Apr 6, 2016

Contributor

Hmm, it's still failing which is odd. Right below this is another example (remoteExecutionByDefault) that only excludes it from db:migrate, so maybe try !Foreman.in_rake?('db:migrate') instead

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Apr 6, 2016

Author Member

added the 'db:migrate', fingers crossed!

This comment has been minimized.

Copy link
@stbenjam

stbenjam Apr 6, 2016

Contributor

That seems to have worked 💚 🎉

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Apr 6, 2016

Author Member

actually I was missing it from here 😄

This comment has been minimized.

Copy link
@stbenjam

stbenjam Apr 6, 2016

Contributor

Ah good catch

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch 4 times, most recently Apr 6, 2016

@daviddavis

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

If a user switches enable_lazy_sync from true to false and the default download policy is "background" that won't be automatically switched to "immediate", trying to figure out how to do that.

The question I have is if a user sets the download_policy to background for a repo and then disables and re-enables lazy sync, will the download_policy still be background?

Also, you might want to talk to pulp to see if there's a way to perform an immediate sync even if the download_policy isn't immediate. I don't think there is but would be good to confirm.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2016

The question I have is if a user sets the download_policy to background for a repo and then disables and re-enables lazy sync, will the download_policy still be background?

@daviddavis the download policy would still be background. Having "enable download policies" turned on and creating a repo with a background download policy, then switching "enable download policies" off, I think it would be ok to assume that the repos created with the setting on still have their original download policies. The only restraints I put are on new repos. One thing we can do is specify in the setting description that enabling or disabling download policies will only affect new repos

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch 2 times, most recently Apr 7, 2016

@daviddavis

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

👍 Code looks good to me. Maybe @ehelms and @stbenjam can re-review.

Will test it out tomorrow and ACK.

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch 2 times, most recently Apr 8, 2016

@daviddavis

This comment has been minimized.

Copy link
Member

commented Apr 8, 2016

@johnpmitsch I noticed that I have to restart my server after changing the setting for the UI to display properly. Are you seeing the same thing?

@@ -33,6 +33,7 @@ class Engine < ::Rails::Engine
),
:config => {
'consumerCertRPM' => consumer_cert_rpm,
'enable_deferred_download_policies' => !Foreman.in_rake? && Setting[:enable_deferred_download_policies],

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 10, 2016

Member

This is the reason @daviddavis is having to restart the application, this is defined at application boot time and not updated when the setting changes. I think your options would be:

  1. Change the value everytime the setting is itself is changed
  2. Query the Settings API from the UI to determine whether this is enabled
  3. Try to make this a proc or lambda to evaluate it at "runtime" aka when the value is sent up on page request

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

I think you can drop this now

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:lazy_sync_ui_disable branch to a6ec67e Apr 12, 2016

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

@ehelms @daviddavis updated, now the UI updates when the setting is changed

data = angular.fromJson(data);
$scope.enableDownloadPolicies = data.results[0].value;
}, function (data) {
ApiErrorHandler.handleGETRequestErrors(data, $scope);

This comment has been minimized.

Copy link
@johnpmitsch

johnpmitsch Apr 12, 2016

Author Member

@ehelms I wasn't sure if this is the correct way to handle an error from the API?

This comment has been minimized.

Copy link
@ehelms
*
* @description
* Provides the functionality for the repository details pane.
*/
angular.module('Bastion.repositories').controller('RepositoryDetailsInfoController',
['$scope', '$state', '$q', 'translate', 'Repository', 'GPGKey', 'CurrentOrganization', 'DownloadPolicy', 'ApiErrorHandler',
function ($scope, $state, $q, translate, Repository, GPGKey, CurrentOrganization, DownloadPolicy, ApiErrorHandler) {
['$scope', '$state', '$q', 'translate', 'Repository', 'GPGKey', 'CurrentOrganization', 'DownloadPolicy', 'ApiErrorHandler', 'BastionConfig', 'Setting',

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Guessing you can drop the requires on BastionConfig here

@@ -36,6 +37,14 @@ angular.module('Bastion.repositories').controller('RepositoryDetailsInfoControll

$scope.progress = {uploading: false};

Setting.get({"search": "name = enable_deferred_download_policies"},
function (data) {

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Can you indent the function calls since they are parameters of the above Setting call?

@@ -36,6 +37,14 @@ angular.module('Bastion.repositories').controller('RepositoryDetailsInfoControll

$scope.progress = {uploading: false};

Setting.get({"search": "name = enable_deferred_download_policies"},
function (data) {
data = angular.fromJson(data);

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Did you find this was required to parse like this? The data didn't come parse already?

@@ -186,10 +186,10 @@ <h4 translate>Basic Information</h4>
</div>

<div class="detail" ng-if="repository.content_type == 'yum'">
<span class="info-label" translate>Download Policy</span>
<span class="info-label" translate> Download Policy</span>

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Unintentional?

<span class="info-value"
bst-edit-select="downloadPolicyDisplay(repository.download_policy)"
readonly="denied('edit_products', product)"
readonly="!(enableDownloadPolicies) || denied('edit_products', product)"

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

The parens aren't necessary

$httpBackend;

beforeEach(module('Bastion.repositories', 'Bastion.test-mocks'));

beforeEach(module({
Setting: {

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

You can do like the following instead of defining this full mock here -- https://github.com/Katello/katello/pull/5939/files#diff-9cef1387b3bc707bc14c38da782722e6R22


beforeEach(inject(function ($injector) {
$httpBackend = $injector.get('$httpBackend');
OstreeBranch = $injector.get('Setting');

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Copy/paste fail?

@@ -26,7 +26,8 @@ def self.load_defaults
self.set('pulp_client_key', N_("Path for ssl key used for pulp server auth"), "/etc/pki/katello/private/pulp-client.key"),
self.set('pulp_client_cert', N_("Path for ssl cert used for pulp server auth"), "/etc/pki/katello/certs/pulp-client.crt"),
self.set('remote_execution_by_default', N_("If set to true, use the remote execution over katello-agent for remote actions"), false),
self.set('use_pulp_oauth', N_("use oauth authentication for pulp instead of the default cert based authentication"), false)
self.set('use_pulp_oauth', N_("use oauth authentication for pulp instead of the default cert based authentication"), false),
self.set('enable_deferred_download_policies', N_("Enable deferred download policies for repositories"), true)

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Do we want this true by default?

@@ -0,0 +1,21 @@
module Katello

This comment has been minimized.

Copy link
@ehelms

ehelms Apr 13, 2016

Member

Could you write a few tests for this to ensure the assumptions being made?

@ehelms

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@daviddavis I didn't test this, just tried to review the new logic in light of the changes to fix your previous (well found) observation.

@daviddavis

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@ehelms thanks for the review. I'll test it tomorrow.

@daviddavis

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

I think the changes in #5969 will suffice in which case we can close out this PR and bug.

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

@daviddavis thanks, closing now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.