Aws dns #9492
Aws dns #9492
Conversation
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
size/L
|
@mazzystr ptal |
michaelgugino
force-pushed
the
aws-dns
branch
4 times, most recently
from
cadbf47
to
a613932
Compare
|
I really like this work. This will be great for us to plug into ! Thanks @michaelgugino ! 👍 |
| loop: "{{ openshift_aws_dns_records | selectattr('private_zone','defined') | map(attribute='private_zone') | list | unique }}" | ||
| loop_control: | ||
| loop_var: l_openshift_aws_route53_scheme | ||
| with_items: "{{ l_zone_items }}" |
There was a problem hiding this comment.
How is a non future proof loop better? See https://docs.ansible.com/ansible/2.6/porting_guides/porting_guide_2.5.html#with-items
| ttl: 300 | ||
| value: "{{ l_openshift_aws_dns_element.value['value'] }}" | ||
| zone: "{{ openshift_aws_dns_zone }}" | ||
| with_dict: "{{ l_openshift_aws_dns_records }}" |
There was a problem hiding this comment.
How is a non future proof loop better? See https://docs.ansible.com/ansible/2.6/porting_guides/porting_guide_2.5.html#with-dict
There was a problem hiding this comment.
Since task "creating record" is now pinned to Route53 zones we will have task & code duplication for Dyn. Dyn is an absolute req to remove the Ops blocker. Again not future proof.
There was a problem hiding this comment.
I was not previously aware of the new loop syntax. I don't see the existing syntax as deprecated.
| # roles/lib_utils/filters/oo_filters.py | ||
| - name: set elb fact dictionary | ||
| set_fact: | ||
| l_openshift_aws_elb_facts: "{{ elb_res | lib_utils_oo_list_of_dict_to_dict_from_key('name')}}" |
There was a problem hiding this comment.
lib_utils_oo_list_of_dict_to_dict_from_key python function wasn't necessary before. Why is this better?
There was a problem hiding this comment.
Previously (your patch set), we had to query ec2 for every record to resolve elb_name to elb_dnsname. That is not great.
This builds a dictionary where we can lookup elb_facts based on elb_name in later tasks or plays.
There was a problem hiding this comment.
Also, the elb facts are not tied to any specific provider, so if we implement providers later, or someone utilizes their custom provider role, they can consume this data.
| @@ -1,22 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
Since task "creating record" is now pinned to Route53 zones we will have task & code duplication for Dyn. Dyn is an absolute req to remove the Ops blocker. Again not future proof.
There was a problem hiding this comment.
There is no dyn requirement. I'm not sure you are following the flow of how the dns records are created.
|
/test gcp |
1 similar comment
|
/test gcp |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelgugino, sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I disagree with the /lgtm
To me this was a lot of work for very little added value to the product. |
No description provided.