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.

Sign up for GitHub

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

Jump to bottom

Remove dependency with mattclay.aws collection #233

Closed
wants to merge 10 commits into from
Closed

Remove dependency with mattclay.aws collection #233

wants to merge 10 commits into from

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Oct 3, 2022 •
edited
Loading

aws/terminator.yml and aws/deploy-test-policy.yml playbooks use mattclay.aws collection, we want to remove this and use directly supported collections community.aws and amazon.aws

@abikouo abikouo force-pushed the remove_community_aws branch 2 times, most recently from 2de34c1 to ba4706f Compare October 6, 2022 14:26
@abikouo abikouo changed the title [WIP][Do not merge] Remove dependency with mattclay.aws collection [WIP] Remove dependency with mattclay.aws collection Oct 6, 2022
@abikouo abikouo changed the title [WIP] Remove dependency with mattclay.aws collection Remove dependency with mattclay.aws collection Oct 17, 2022
@abikouo abikouo force-pushed the remove_community_aws branch from a8b38ac to 44cfe64 Compare October 17, 2022 13:01
gravesm
gravesm reviewed Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

I am not sure that we need to prioritize removing the dependency on mattclay.aws. If there are modules in the supported amazon.aws collection that provide the exact same functionality as modules in mattclay.aws, we can look at replacing those. We shouldn't be vendoring copies of modules from mattclay.aws just to remove that dependency, though.

@abikouo
Copy link
Contributor Author

abikouo commented Oct 17, 2022

I am not sure that we need to prioritize removing the dependency on mattclay.aws. If there are modules in the supported amazon.aws collection that provide the exact same functionality as modules in mattclay.aws, we can look at replacing those. We shouldn't be vendoring copies of modules from mattclay.aws just to remove that dependency, though.

This is exactly what has been done. The mattclay.aws modules have been replaced by those from supported collection with the same feature. Nothing has been copied, terminator_lambda_package.py was copied by can be replaced by the community.general.archive module doing the same

@gravesm
Copy link
Collaborator

gravesm commented Oct 17, 2022

Nothing has been copied, terminator_lambda_package.py was copied

This is my problem. Why copy anything at all? There is no pressing need to remove the dependency on mattclay.aws.

@abikouo
Copy link
Contributor Author

abikouo commented Oct 17, 2022

Nothing has been copied, terminator_lambda_package.py was copied

This is my problem. Why copy anything at all? There is no pressing need to remove the dependency on mattclay.aws.

I will turn terminator_lambda_package into community.general.archive

@abikouo abikouo requested a review from gravesm October 18, 2022 16:18
@gravesm gravesm force-pushed the remove_community_aws branch from d3dff6a to 3ee2227 Compare December 2, 2022 15:33
gravesm
gravesm reviewed Dec 2, 2022
View reviewed changes
Copy link
Collaborator

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

I went ahead and rebased this. Sorry for the delay. There are a few small things that need addressing, but otherwise it looks good.

mattclay
mattclay requested changes Dec 2, 2022
View reviewed changes
aws/terminator.yml Outdated
- name: package terminator requirements
tags: lambda
lambda_package:
src: "{{ packaging_dir }}/terminator-requirements"
archive:
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't a full replacement for the lambda_package module. The generated archive changes based on the timestamps of the included files, which results in a new Lambda version being created even when the file contents are unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
Idempotency check for the lambda layer archive is assumed by downloading the one stored in AWS and compare it with the local one in a deterministic way (setting a constant time value)

Copy link
Owner

Choose a reason for hiding this comment

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

Why add a custom module to do the comparison instead of using the existing module to create the archive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

existing I did not found a way in the module to create archive in a deterministic way, so we will always have a diff between the sha from aws and the local one, please suggest if there is a way to solve that

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I wasn't specific enough. I was asking why replace mattclay.aws.lambda_package?

Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for wanting to remove the dependency on the mattclay.aws collection to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the intend was to migrate to a collection supported by the cloud team.
@jillr could you please clarify

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the intend was to migrate to a collection supported by the cloud team. @jillr could you please clarify

This was never the intent. There was a need to update community.aws, in particular migrating to the newer iam_role module. There's never been a problem with the mattclay.aws collection. If there are modules from the supported amazon.aws collection that can fully replace modules from the mattclay.aws collection, we can consider those, but that is not apparently the case here with the lambda_package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then I will keep lambda_package from mattclay.aws

Copy link
Owner

Choose a reason for hiding this comment

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

FYI - I've updated the mattclay.aws collection to version 3.0.0. It should now play nice with amazon.aws 5.x.

@abikouo abikouo force-pushed the remove_community_aws branch from 3ee2227 to 8b9ac44 Compare December 5, 2022 16:43
@gravesm
Copy link
Collaborator

gravesm commented Feb 14, 2023

I would like to either close this PR or get it to a state that it can be merged. With mattclay.aws upgraded to now work with newer versions of amazon.aws and community.aws, I'm not sure the changes here are necessary. There was a need to move off of older, unsupported modules, but that's no longer a problem.

@abikouo abikouo closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattclay mattclay mattclay requested changes

@gravesm gravesm gravesm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abikouo @gravesm @mattclay