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

New module: elb_target_group_facts (clount/amazon/elb_target_group_facts) #24583

Merged
merged 1 commit into from Jun 14, 2017

Conversation

Projects
None yet
7 participants
@wimnat
Copy link
Contributor

commented May 14, 2017

SUMMARY

New module elb_target_group_facts

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

elb_target_group_facts

ANSIBLE VERSION

2.4

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2017

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/cloud/amazon/elb_target_group_facts.py:64:1: E311 EXAMPLES is not valid YAML

click here for bot help

@wimnat wimnat force-pushed the wimnat:feature/elb_target_group_facts branch to 1798a50 May 14, 2017

@bcoca bcoca removed the needs_triage label May 15, 2017

description:
- The Amazon Resource Names (ARN) of the target groups.
required: false
names:

This comment has been minimized.

Copy link
@ryansb

ryansb May 16, 2017

Contributor

For consistency, can you make this target_group_names instead?

target_group['tags'] = get_target_group_tags(connection, module, target_group['TargetGroupArn'])

# Turn the boto3 result in to ansible_friendly_snaked_names
snaked_target_groups = [camel_dict_to_snake_dict(target_group) for target_group in target_groups['TargetGroups']]

This comment has been minimized.

Copy link
@ryansb

ryansb May 16, 2017

Contributor

Same as the ALB facts module: can you add a way to get items by name/arn out of the returned facts?

@alikins alikins changed the title New module elb_target_group_facts New module: elb_target_group_facts (clount/amazon/elb_target_group_facts) May 22, 2017

@ansibot ansibot added the stale_ci label May 22, 2017

@willthames

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

I could really use @ryansb's suggestions too. Let me know if a PR against your branch would help

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

I've been wondering if this is better done with a filter. I can understand the use case and it could be applied to other facts modules.

Thoughts?

@willthames

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

I'm not against the idea of a filter, just not sure how easy it would be to come up with something suitably generic and reusable.

Something like this?:

target_groups|group_by('target_group_arn')['arn_of_interest']
@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

Yeh pretty much. All AWS facts modules will return you a list of dicts so all you need to do is provide the dict key you want to send back as the 'grouper'.

This then means you can group by any key you want and apply to any of the existing facts modules

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

Jinja already has groupby. I'm playing to see if this can be used else I'll create a new filter.

@wimnat

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2017

@ryansb how do you feel about the filter suggestion?

@ryansb

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2017

The groupby filter seems to be exactly right - would it be possible for you to add that to the examples for this module, so it's easy for people to discover?

@ryansb

ryansb approved these changes Jun 9, 2017

try:
import boto3
from botocore.exceptions import ClientError, NoCredentialsError
HAS_BOTO3 = True

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 13, 2017

Contributor

could add HAS_BOTO3 to the imports from ansible.module_utils.ec2 instead of manually setting

- Gather facts about ELB target groups in AWS
version_added: "2.4"
author: Rob White (@wimnat)
options:

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 13, 2017

Contributor

could add requirements to the docs

target_groups = target_group_paginator.paginate(Names=names).build_full_result()
except ClientError as e:
if e.response['Error']['Code'] == 'TargetGroupNotFound':
module.exit_json(target_groups=[])

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 13, 2017

Contributor

Is there another option than this?
If I give the parameters:

      elb_target_group_facts:
        names:
          - validtargetgroup
          - notatargetgroup

and validtargetgroup exists and notatargetgroup doesn't, I'll get back no facts. Not sure if that's desired behavior or easily fixed if not.

This comment has been minimized.

Copy link
@s-hertel

s-hertel Jun 13, 2017

Contributor

You can disregard the above - I've seen the discussion in your elb_application_lb_facts PR and it makes sense why you've done this.

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2017

The suggestions I had aren't vital and can be a followup PR.

shipit

@ansibot ansibot removed the stale_ci label Jun 13, 2017

Will be addressed in followup PR

@ryansb ryansb merged commit f8d027b into ansible:devel Jun 14, 2017

1 check passed

Shippable Run 25382 status is SUCCESS.
Details

@wimnat wimnat deleted the wimnat:feature/elb_target_group_facts branch Jun 14, 2017

@alikins

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

#22935 seems resolved by this.

@ansible ansible locked and limited conversation to collaborators Apr 26, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.