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 #26264, #25721 - Detect variable types on import #252

Merged
merged 1 commit into from Mar 29, 2019

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Mar 7, 2019

No description provided.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, few small suggestions

end

def fetch_vars(key)
params[:changed] ? (params[:changed][key].as_json || {}) : {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you touch this already, params.fetch(:changed, {}).fetch(key).try(:as_json) || {} is I think equivalent and slightly safer (in case params[:changed] is not a hash)

@@ -96,5 +99,25 @@ def local_variables
def remote_variables
proxy_api.all_variables
end

def infer_key_type(value)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we extract this to a constant in case we'll need to use it elsewhere too?

Float => 'real',
Array => 'array',
Hash => 'hash'
}.reduce('string') { |memo, (key, type)| value.is_a?(key) && memo == 'string' ? type : memo }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something, but can't we add String => 'string' to the hash and then just do hash[value.class] || 'string'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

13.class #=> Fixnum

I had that previously, rubocop was complaining I should be using Integer instead of Fixnum. Not sure which one is better, let me know your preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, this would be the pattern

{
'Fixnum' => 'integer',
'Integer' => 'integer'
}
h[value.class.to_s] || 'string'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made updates as suggested.

@ares
Copy link
Member

ares commented Mar 26, 2019

I think it changes with this PR, when I modify the default value of the variable and reimport variables. It shows up as updated, if I mark that for import, it reset the default value. That is probably undesired behavior.

@xprazak2
Copy link
Contributor Author

I made changes as suggested, default value is no longer updated for the variables that already exist. This means that when users want to update variables using import, they need to delete them in Foreman and newly import.

@ares
Copy link
Member

ares commented Mar 28, 2019

rubocop failure

.....................................................C................................................C

Offenses:

/home/jenkins/workspace/foreman_ansible-pull-request/database/postgresql/label/fast/ruby/2.5/plugin/app/services/foreman_ansible/variables_importer.rb:52:9: C: Style/MultilineIfModifier: Favor a normal if-statement over a modifier clause in a multiline statement.
        variable.assign_attributes(:default_value => variable_default, ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/jenkins/workspace/foreman_ansible-pull-request/database/postgresql/label/fast/ruby/2.5/plugin/test/unit/services/ansible_variables_importer_test.rb:15:50: C: Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
      new_role.name => { 'new_var' => 'new value'},
                                                 ^

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thanks @xprazak2, merging!

@ares ares merged commit b3dc2d7 into theforeman:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants