Your wish is my command... #6
Conversation
NickLaMuro
force-pushed
the
ansible_credential_translation_mixin
branch
from
fdcb211
to
5f1d8d2
Compare
| # Translates options hash with credential_ids into auth | ||
| # | ||
| # @param [Hash] options Hash containing the credential_ids | ||
| # @param [Hash] other_hash (options) Data to be updated if differs from <options> |
There was a problem hiding this comment.
# @param [Hash] other_hash ...
#namingIsHard™
| AUTH_TYPES.each do |credential| | ||
| credential_sym = "#{credential}_id".to_sym | ||
| credential_id = mutate_options ? options.delete(credential_sym) : options[credential_sym] | ||
| other_hash[credential] = Authentication.find(credential_id).native_ref if credential_id.present? |
There was a problem hiding this comment.
I think I covered all of the corner cases with this by using :other_hash and :mutate_options, but please check my work. I already forgot to assign this to other_hash instead of options.
Probably want to test this mixin directly and heavily just in case someone comes and changes this method and does something unexpected.
|
|
||
| module_function | ||
|
|
||
| # Translates options hash with credential_ids into auth |
There was a problem hiding this comment.
Along the same lines with my comment about adding specs, the docs on this probably could be fleshed out a bit too.
| @@ -2,6 +2,8 @@ class ServiceTemplateAnsiblePlaybook < ServiceTemplateGeneric | |||
| before_destroy :check_retirement_potential, :prepend => true | |||
| around_destroy :around_destroy_callback, :prepend => true | |||
|
|
|||
| include AnsibleRunnerAuthTranslations | |||
There was a problem hiding this comment.
Fudged this one up... this needs to be in the class method space, so it should have been a extend, not include. Fixing...
| module AnsibleRunnerAuthTranslations | ||
| AUTH_TYPES = %i[credential vault_credential cloud_credential network_credential] | ||
|
|
||
| module_function |
There was a problem hiding this comment.
By making this method a module_function, this can be called in two ways:
- By
includeing it, and calling it as we have done currently (as if it were a method in the class) - Calling it from it's
moduledirectly as if it was a class method (similar toFileUtils)
The second form would look like:
AnsibleRunnerAuthTranslations.translate_credentials(options)Which has the advantage of not adding methods to the classes we are writing this for, though, when adding a module that has module_functions, it should add them into the private space (I think...).
DRYs up code for translating credentials. Makes it as flexible as possible to fit into the multiple places it is being used.
NickLaMuro
force-pushed
the
ansible_credential_translation_mixin
branch
from
5f1d8d2
to
d262dbb
Compare
|
So I was thinking more along the lines of someone from GM's team helping to work out how we can dedup the whole processing in this part of the code base. I don't really want to create a top-level mixin for one method that, realistically, I would like to get rid of entirely in time (there's no reason the change the keys since we're not passing them to tower anymore). |
|
Fair enough. My bad for misinterpreting. Feel free to close then, since I think your broader plan makes more sense long term. |
@carbonin As you requested 😄
DRYs up code for translating credentials. Makes it as flexible as possible to fit into the multiple places it is being used.
Note: commit is
[WIP]until I can run specs for the previous areas of the code to confirm this at least works a little bit.Links