Fixes #23536 - No errors when unattended=false #5687

shiramax · 2018-06-13T08:10:33Z

Running 'rake apipie:cache:index' is no longer results in NoMethodError when unattended=false is set in the settings.yaml file.

theforeman-bot · 2018-06-13T08:10:39Z

Issues: #23536

theforeman-bot · 2018-06-13T08:10:40Z

There were the following issues with the commit message:

commit message for 78a8fcf is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2018-06-13T08:31:52Z

There were the following issues with the commit message:

commit message for b9bd1dc is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2018-06-13T08:45:00Z

There were the following issues with the commit message:

commit message for bb6c3c5 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

xprazak2 · 2018-06-13T12:41:52Z

Takes care of the issue for me. Could you please modify the commit message as the bot suggests?

Test failures are unrelated.

tbrisker · 2018-06-14T14:03:52Z

@ShimShtein @lzap since i think you're most familiar with orchestration code, does it make sense to include orchestration module when unattended==false or would that lead to unexpected results?

lzap · 2018-06-15T09:06:55Z

I think this is safe, but let's pause this for a moment and talk about removing this flag for once and forever:

https://community.theforeman.org/t/rfc-remove-unattended-setting/10035

shiramax · 2018-06-17T10:46:23Z

@lzap thanks, I agree we need to re-think about the unattended mode,
I predict it will take some time until the unattended mode will be removed completely so I think that in the meantime it's best to add this PR.

lzap

On second look, I think that safer approach is to simply use respond_to? in valid_rebuild_only_values and skip it when orchestration is not there.

I will let @iNecas comment on that, this is something from apipie and I don't exactly know what are these method lists used for.

This kind of change we can hardly cover with unit tests, we turn orchestration off for most of them (and orchestration has separate testing) @ohadlevy - only if we had end to end automated testing, that would do it.

iNecas · 2018-06-22T15:23:27Z

As the backtrace shows, the methods are used from here

foreman/config/initializers/apipie.rb

Line 24 in d7c6774

    
           :host_rebuild_steps => -> { Host::Managed.valid_rebuild_only_values.join(', ') }

, and they are used to generate values to be used in translations of docs strings of the apipie (such as here

foreman/app/controllers/api/v2/hostgroups_controller.rb

Line 110 in 630061d

    
           param :only, Array, :desc => N_("Limit rebuild steps, valid steps are %{host_rebuild_steps}")

).

I agree using respond_to at corresponding place is safer to go ATM.

shiramax · 2018-06-24T13:02:28Z

@iNecas @lzap @tbrisker @xprazak2 thanks, please review again :)

iNecas

Tested and I can confirm it fixes the issue. The test failures are unrelated. Letting somebody else involved in the review to merge it, just in case there was more to do here.

tbrisker · 2018-06-25T09:27:06Z

app/models/host/managed.rb

@@ -507,7 +508,7 @@ def self.registered_provision_methods
  end

  def self.valid_rebuild_only_values
-    Nic::Managed.rebuild_methods.values + Host::Managed.rebuild_methods.values
+    Nic::Managed.rebuild_methods.values + Host::Managed.rebuild_methods.values if Host::Managed.respond_to?(:rebuild_methods)


not sure if this is on purpose or not, but the if clause here applies to the whole line. Perhaps return nil early instead to make it clearer this is intentional?
does it make sense to only return Nic::Managed.rebuild_methods.values if unattended=false or not? since orchestration is included in Nic::Managed always, that method will work.

tbrisker · 2018-06-25T09:34:22Z

config/initializers/apipie.rb

@@ -21,7 +21,7 @@
    :providers_requiring_url => -> { ComputeResource.providers_requiring_url },
    :default_nic_type => InterfaceTypeMapper::DEFAULT_TYPE.humanized_name.downcase,
    :template_kinds => -> { Rails.cache.fetch("template_kind_names", expires_in: 1.hour) {TemplateKind.pluck(:name).join(", ")} },
-    :host_rebuild_steps => -> { Host::Managed.valid_rebuild_only_values.join(', ') }
+    :host_rebuild_steps => (-> { Host::Managed.valid_rebuild_only_values.join(', ') } if Host::Managed.valid_rebuild_only_values)


you can use Host::Managed.valid_rebuild_only_values&.join(', ') instead of the condition to handle nil as well without calling the function twice

shiramax · 2018-06-25T10:59:48Z

@tbrisker thanks,
please review again :)

tbrisker · 2018-06-25T11:54:39Z

config/initializers/apipie.rb

@@ -21,7 +21,7 @@
    :providers_requiring_url => -> { ComputeResource.providers_requiring_url },
    :default_nic_type => InterfaceTypeMapper::DEFAULT_TYPE.humanized_name.downcase,
    :template_kinds => -> { Rails.cache.fetch("template_kind_names", expires_in: 1.hour) {TemplateKind.pluck(:name).join(", ")} },
-    :host_rebuild_steps => -> { Host::Managed.valid_rebuild_only_values.join(', ') }
+    :host_rebuild_steps => (-> { Host::Managed.valid_rebuild_only_values.join(', ') } || nil)


valid_rebuild_only_values will always return an array now so the nil will never execute

shiramax · 2018-06-26T07:27:36Z

thanks @tbrisker , it should be fine now.

tbrisker

Thanks @shiramax !

lzap · 2018-06-26T08:31:42Z

Terrible hack but thumbs up. I would love to kill unattended setting, but we need to plan this we still have users of this.

stale

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Jun 13, 2018

shiramax force-pushed the 23536 branch from 78a8fcf to b9bd1dc Compare June 13, 2018 08:31

theforeman-bot added Needs testing and removed Waiting on contributor labels Jun 13, 2018

theforeman-bot added the Waiting on contributor label Jun 13, 2018

shiramax changed the title ~~Fixes #23536 - no error when unattended=false~~ Fixes #23536 - No errors when unattended=false Jun 13, 2018

shiramax changed the title ~~Fixes #23536 - No errors when unattended=false~~ Fixes #23536 - No errors when unattended=false Jun 13, 2018

shiramax force-pushed the 23536 branch from b9bd1dc to bb6c3c5 Compare June 13, 2018 08:44

theforeman-bot added Needs testing and removed Waiting on contributor labels Jun 13, 2018

theforeman-bot added the Waiting on contributor label Jun 13, 2018

shiramax force-pushed the 23536 branch from bb6c3c5 to 1832a10 Compare June 13, 2018 08:47

theforeman-bot added Needs testing and removed Waiting on contributor labels Jun 13, 2018

shiramax force-pushed the 23536 branch from 1832a10 to f5f3895 Compare June 13, 2018 12:49

lzap previously requested changes Jun 20, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jun 20, 2018

shiramax force-pushed the 23536 branch from f5f3895 to 2b17755 Compare June 24, 2018 13:00

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 24, 2018

iNecas previously approved these changes Jun 25, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 25, 2018

tbrisker suggested changes Jun 25, 2018

View reviewed changes

shiramax dismissed iNecas’s stale review via 8831daf June 25, 2018 10:58

shiramax force-pushed the 23536 branch from 2b17755 to 8831daf Compare June 25, 2018 10:58

tbrisker suggested changes Jun 25, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Jun 25, 2018

shiramax force-pushed the 23536 branch from 8831daf to bf879fd Compare June 26, 2018 06:52

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 26, 2018

Fixes #23536 - No errors when unattended=false

b0d8eb1

shiramax force-pushed the 23536 branch from bf879fd to b0d8eb1 Compare June 26, 2018 07:26

tbrisker approved these changes Jun 26, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 26, 2018

tbrisker merged commit c189204 into theforeman:develop Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #23536 - No errors when unattended=false #5687

Fixes #23536 - No errors when unattended=false #5687

shiramax commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

xprazak2 commented Jun 13, 2018

tbrisker commented Jun 14, 2018

lzap commented Jun 15, 2018

shiramax commented Jun 17, 2018

lzap left a comment

iNecas commented Jun 22, 2018

shiramax commented Jun 24, 2018

iNecas left a comment

tbrisker Jun 25, 2018

tbrisker Jun 25, 2018

shiramax commented Jun 25, 2018

tbrisker Jun 25, 2018

shiramax commented Jun 26, 2018

tbrisker left a comment

lzap commented Jun 26, 2018

Fixes #23536 - No errors when unattended=false #5687

Fixes #23536 - No errors when unattended=false #5687

Conversation

shiramax commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

theforeman-bot commented Jun 13, 2018

xprazak2 commented Jun 13, 2018

tbrisker commented Jun 14, 2018

lzap commented Jun 15, 2018

shiramax commented Jun 17, 2018

lzap left a comment

Choose a reason for hiding this comment

iNecas commented Jun 22, 2018

shiramax commented Jun 24, 2018

iNecas left a comment

Choose a reason for hiding this comment

tbrisker Jun 25, 2018

Choose a reason for hiding this comment

tbrisker Jun 25, 2018

Choose a reason for hiding this comment

shiramax commented Jun 25, 2018

tbrisker Jun 25, 2018

Choose a reason for hiding this comment

shiramax commented Jun 26, 2018

tbrisker left a comment

Choose a reason for hiding this comment

lzap commented Jun 26, 2018