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

adding AWS Glue Job actions to test policies #95

Closed
wants to merge 10 commits into from
Closed

adding AWS Glue Job actions to test policies #95

wants to merge 10 commits into from

Conversation

cdschneider
Copy link

per @tremble comments on PR ansible-collections/community.aws#57

Adding glue: permissions to test policies in order to support new aws_glue_job module coverage in integration tests.

Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this on our staging environment with community.aws/pull/57, couple of small things here and one note I'll leave on that PR. Thanks for this!

aws/policy/application-services.yaml Outdated Show resolved Hide resolved
aws/policy/application-services.yaml Outdated Show resolved Hide resolved
@cdschneider cdschneider requested a review from jillr May 16, 2020 16:16
@jillr
Copy link
Collaborator

jillr commented Jul 9, 2020

@cdschneider The changes you have here are good, however the merge conflict is a bit hairy, partially because of #97 and partially I think from the merge commit. If you don't mind me pushing to your branch I can try to get those sorted out in the next few days.

@cdschneider
Copy link
Author

I don't mind at all! @jillr

@tremble
Copy link
Contributor

tremble commented Sep 9, 2020

@jillr Merge looks to have gone a little wonky (@cdschneider sorry, #97 was pretty major surgery, but should massively simplify updating the policies)

@cdschneider
Copy link
Author

If it's easier, I'm happy to close this and re-open a new PR with my changes starting with a fresh branch @jillr @tremble

@jillr
Copy link
Collaborator

jillr commented Jan 14, 2021

@cdschneider Sorry for the late reply, however you prefer to resolve the merge conflict works for me.

Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cdschneider, the policies look good but we'll also need a new class added to the terminator lambda for Glue Jobs. The terminator is a lambda function that runs in our CI account and deletes resources that have been online for longer than a set time, in case a CI job fails to clean up after itself for some reason.

You can see an example of the class for cleaning up glue connections here:
https://github.com/mattclay/aws-terminator/blob/master/aws/terminator/data_services.py#L54-L72

ETA: my apologies, I have too many tabs open - this comment was for 137!

@cdschneider cdschneider requested a review from jillr March 19, 2021 02:24
@cdschneider
Copy link
Author

OK @jillr I think that I've addressed all your previous comments, fixed outstanding merge conflicts, and added the GlueJob class to the data_services.py to be used by the terminator lambda. Is there anything else outstanding on this PR?

@cdschneider
Copy link
Author

@jillr Can you please let me know if this PR is still needed, or if #137 is a replacement for this work? Sorry for my slow-responses on addressing the changes you requested, but all requested changes should all be addressed now in this PR (if it is still needed).

@tremble
Copy link
Contributor

tremble commented Sep 20, 2021

Looking at the policies added in this PR the Glue actions look they were added in #137 and we should be able to close this PR out.

@jillr
Copy link
Collaborator

jillr commented Sep 27, 2021

I think tremble is right, community.aws #57 needs a rebase because we no longer use shippable (JFrog discontinued the service) and the PR needs to pick up the new CI config from main but I believe we have the policies we need.

@jillr jillr closed this Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants