f5devcentral / ansible-role-f5_atc_deploy_declaration

Ansible role used to deploy declaratives to F5 Automated Tool Chain services: AS3, DO, and TS
Apache License 2.0
7 stars 11 forks source link

Few minor corrections #1

Closed crosbygw closed 4 years ago

crosbygw commented 4 years ago

Adding in edits from Forrest:

Many var names seem to have act instead of atc

lol, no idea why. Will make this change.

We should default to async true if AS3(maybe not even offer without async)

Agree, I will remove as an option.

We should standardize how vars are introduced to our roles

Use provider as something that needs to be passed to the role instead of creating it based on vars passed.

The reason for this is that right now, each role has unique vars named specifically for it. If you called 2 roles in the same playbook, you would need to duplicate passing in user/pass/transport, etc

Which role should I use as an example for standard?

For service types, I think we should use AS3, DO, and TS instead of "device" and "telemetry". Just for consistency

The reason I used device and telemetry is I am using the class key defended in the declaration to determine the service and the class for DO is 'device' and the class for ts is 'telemetry'. I am going to guess the class was defined before the marketing name for the service was finalized.

 For act_task_check.yaml we should make the retry and delay timing configurable by the user but leave the defaults that you have

Agree

We need a way to read the output of AS3/DO/TS and notify the playbook if there was a change or no change based on the output. If an AT tool sees no change, it will have that in the payload. Right now change status seems dependent on if it downloads a file or not.

Do all the tools report a change? I will look to add in logic to detect when a change occurs.

Do you have to set act_declaration_file and act_declaration_url? Seems like it should be either or?

So this one was a little frustrating for me. I tried to use the ansible lookup plug-in lookup(url), but when i added it as the method to grab json payload it would crash python on my system. I filled a bug but in the meantime went another route to get something out for preview. Instead i am using get_url which requires a source and a destination. atc_declaration (required) is the file that is read and posted to service, while atc_declaration_url is optional and is used to create the file defined in act_declaration_file.

We probably shouldn’t default method to POST since that’s a change, vs GET which is read

Agree

Few tasks that need to change how they evaluate the condition to avoid deprecation notice:

[DEPRECATION WARNING]: evaluating show_hash as a bare variable, this behaviour will go away and you might need to add |bool to the expression in the future. Also see

CONDITIONAL_BARE_VARS configuration toggle.. This feature will be removed in version 2.12. Deprecation warnings can be disabled by setting deprecation_warnings=False in

ansible.cfg.

Agree