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 for AWS CodeBuild #47187

Open
wants to merge 23 commits into
base: devel
from

Conversation

@stefanhorning
Contributor

stefanhorning commented Oct 17, 2018

SUMMARY

New module aws_codebuild for the AWS CodeBuild service.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aws_codebuild.yml module

@stefanhorning

This comment has been minimized.

Contributor

stefanhorning commented Oct 17, 2018

Sucessor of PR #38943

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 17, 2018

Hi @stefanhorning, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 17, 2018

@stefanhorning, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 17, 2018

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:290:0: ImportError: No module named botocore

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:290:0: ImportError: No module named botocore

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:290:0: ImportError: No module named 'botocore'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:290:0: ModuleNotFoundError: No module named 'botocore'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:290:0: ModuleNotFoundError: No module named 'botocore'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/aws_codebuild.py:292:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/aws_codebuild.py:413:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/aws_codebuild/defaults/main.yml:3:1: empty-lines too many blank lines (1 > 0)

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 17, 2018

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Madhura-CSI @MichaelBaydoun @Sodki @adq @akazakov @alachaum @amir343 @anryko @bekelchik @bpennypacker @brandond @chenl87 @defunctio @dennisconrad @dkhenry @fiunchinho @fivethreeo @flowerysong @garethr @gunzy83 @gurumaia @hsingh @hyperized @iiibrad @infectsoldier @j-carl @jarv @Java1Guy @jimbydamonk @jmenga @joelthompson @jonhadfield @jonmer85 @joshsouza @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @michaeljs1990 @minichate @mjschultz @mmochan @nadirollo @nand0p @naslanidis @nathanwebsterdotme @nerzhul @nickball @orthanc @piontas @pjodouin @prasadkatti @psykotox @pwnall @raags @rickmendes @roadmapper @ryansydnor @scicoin-project @scottanderson42 @sdubrul @shepdelacreme @silviud @slapula @steynovich @tastychutney @tedder @tgerla @timmahoney @tombamford @tsiganenok @viper233 @whiter @willricardo @wilvk @wimnat @yaakov-github @zacblazic @zbal @zeekin @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@stefanhorning

This comment has been minimized.

Contributor

stefanhorning commented Oct 17, 2018

Ready to merge from my side (I an do a rebase once reviewed, if there is a wish)

try:
import botocore
except ImportError:
pass # will be detected by imported HAS_BOTO3

This comment has been minimized.

@flowerysong

flowerysong Oct 17, 2018

Contributor
Suggested change Beta
pass # will be detected by imported HAS_BOTO3
pass # Handled by AnsibleAWSModule
name = params['name']
# Sanity check and cleanup params not needed for boto methods:
if not isinstance(name, str):
module.fail_json(msg="Params was missing name", exception=traceback.format_exc())

This comment has been minimized.

@flowerysong

flowerysong Oct 17, 2018

Contributor

This looks suspicious; I don't see any source for an exception, so you shouldn't be trying to pass one to fail_json. This check is also unnecessary, since the 'name' argument is marked as required in the argspec.

return resp, changed
except botocore.exceptions.ClientError as e:
module.fail_json(msg="Unable create project {0}: {1}".format(name, to_native(e)),
exception=traceback.format_exc(), **camel_dict_to_snake_dict(e.response))

This comment has been minimized.

@flowerysong

flowerysong Oct 17, 2018

Contributor

This (and all of the other exception handling) should be using module.fail_json_aws() instead of doing manual exception formatting.

region: "{{ aws_region }}"
no_log: yes
- name: create IAM role needed for CodeBuild

This comment has been minimized.

@flowerysong

flowerysong Oct 17, 2018

Contributor

This role should probably be cleaned up after the tests are done. (Also, I don't have any experience specifically with CodeBuild, but based on general experience with AWS you may need to insert a short pause after the IAM role is created to avoid sporadic failures when trying to use a newly created role.)

This comment has been minimized.

@stefanhorning

stefanhorning Oct 19, 2018

Contributor

Done

stefanhorning added some commits Oct 17, 2018

Cleanup after testing aws_codebuild module. Also add a pause after ia…
…m_role creation to make sure role is available for use.
@stefanhorning

This comment has been minimized.

Contributor

stefanhorning commented Oct 17, 2018

@flowerysong applied all your recommended changes now.

@ptux

All cleanup tasks should be done in always block to avoid that cleanup does not execute when failed.

<<: *aws_connection_info
async: 300
# ============================== cleanup ======================================

This comment has been minimized.

@ptux

ptux Oct 18, 2018

Contributor

All cleanup tasks should be done in always block to avoid that cleanup does not execute when failed.

@@ -0,0 +1,2 @@
cloud/aws
unsupported

This comment has been minimized.

@ptux

ptux Oct 18, 2018

Contributor

You are turning the integration tests off.
So, the integration tests you wrote are not executed.

@jborean93 jborean93 removed the needs_triage label Oct 18, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 18, 2018

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/aws_codebuild/aliases:0:0: missing alias `shippable/aws/group[1-2]` or `unsupported`

click here for bot help

@ansibot ansibot added the ci_verified label Oct 18, 2018

stefanhorning added some commits Oct 19, 2018

@ansibot ansibot removed the ci_verified label Oct 19, 2018

- name: create IAM role needed for CodeBuild
iam_role:
name: "{{ resource_prefix }}-codebuild-service-role"

This comment has been minimized.

@stefanhorning

stefanhorning Oct 19, 2018

Contributor

The CI testrunner fails to create this IAM role (AccessDenied), so what do I need to do to get this working?

This comment has been minimized.

@mattclay

mattclay Nov 5, 2018

Member

We'll need to update our CI system to accommodate the additional permissions required.

type: LINUX_CONTAINER
environment_variables:
- { name: 'FOO_ENV', value: 'other' }
region: "{{ aws_region }}"

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

This is already provided in the credentials so it causes a warning for the duplicate key.

type: LINUX_CONTAINER
environment_variables:
- { name: 'FOO_ENV', value: 'other' }
region: "{{ aws_region }}"

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

Another duplicate region parameter

clean_params.pop('region', None)
clean_params.pop('state', None)
clean_params.pop('validate_certs', None)
formatted_params = snake_dict_to_camel_dict(clean_params)

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

I'd like to have a whitelist of expected parameters, rather than popping off those that are unexpected. This is buggy right now since you can pass in unsupported params (such as aws_access_key). If other parameters are ever added to the module or doc fragments this could continue causing possible issues. If you don't want to hardcode your whitelist you can make it dynamic:

from ansible.module_utils.aws.core import get_boto3_client_method_parameters

formatted_params = snake_dict_to_camel_dict(dict((k, v) for k, v in params.items() if v is not None))

permitted_update_params = get_boto3_client_method_parameters(client, 'update_project')
permitted_create_params = get_boto3_client_method_parameters(client, 'create_project')

formatted_clean_update_params = dict((k, v) for k, v in formatted_params.items() if k in permitted_update_params)
formatted_clean_create_params = dict((k, v) for k, v in formatted_params.items() if k in permitted_create_params)

This comment has been minimized.

@stefanhorning

stefanhorning Nov 13, 2018

Contributor

Yes, that was still my first hacky solution, thanks for catching that. Also didn't know there was a convinient way already to determine valid param. Updated now

- name: Wait until IAM role is available
pause:
seconds: 3

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

This delay is too short so the tests are repeatedly failing for me

aws_codebuild:
name: "{{ output.project.name }}"
state: absent
<<: *aws_connection_info

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

source and artifacts are required by the module so this task fails

This comment has been minimized.

@stefanhorning

stefanhorning Nov 13, 2018

Contributor

Fixed all your comments except this one for now. Will wait until the CI runner is updated (to create the IAM role), so I can iron out any remaining issues with the tests.

packaging: NONE
type: CODEPIPELINE
name: test
encryption_key: 'arn:aws:kms:{{ aws_region }}:{{ aws_account_id }}:alias/aws/s3'

This comment has been minimized.

@s-hertel

s-hertel Nov 13, 2018

Contributor

aws_account_id is undefined

This comment has been minimized.

@stefanhorning

stefanhorning Nov 13, 2018

Contributor

This should also be fixed now

@ansibot ansibot removed the stale_ci label Nov 13, 2018

@stefanhorning

This comment has been minimized.

Contributor

stefanhorning commented Nov 16, 2018

ready_for_review

@ansibot ansibot added the stale_ci label Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment