Skip to content

fix(event_sources): Update CodePipeline event source to include optional encryption_key field and make user_parameters field optional#2113

Merged
leandrodamascena merged 5 commits into
aws-powertools:developfrom
neilramsay:fix/codepipeline_event_optional_user_params
Apr 12, 2023
Merged

fix(event_sources): Update CodePipeline event source to include optional encryption_key field and make user_parameters field optional#2113
leandrodamascena merged 5 commits into
aws-powertools:developfrom
neilramsay:fix/codepipeline_event_optional_user_params

Conversation

@neilramsay

@neilramsay neilramsay commented Apr 12, 2023 •
edited by leandrodamascena
Loading

Copy link
Copy Markdown
Contributor

Issue number: #2117

Summary

The AWS CodePipeline Lambda Action does not require UserParametersto be set, and therefore will be missing from the action_configuration.
https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-Lambda.html

Changes

Make CodePipelineJobEvent user_parameters Optional (as in Python typing optional)

User experience

Prior to change, calling event.user_parameters would result in an exception due to
the "UserParameters" key not being in the Action Configuration data class.

Post change, this will return None. When using Python type checking, this will inform the developer they need to handle the case where the User Parameters are not present.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

Not explicitly; runtime wise, this will not break any existing code that works.
It will create additional errors for those performing type checking, and haven't handled this case.

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@neilramsay
test: CodePipeline Event with missing User Params
7dd03a8 
The AWS CodePipeline Lambda Action does not require UserParameters
to be set, and therefore will be missing from `action_configuration`.
This test ensures the data class returns None.

https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-Lambda.html
@neilramsay
test: CodePipeline Event with non-JSON User Params
33375f0 
The User Parameters supplied can be arbitary text.
This test ensures a JSON parse exception is raised if plain
text is supplied as a User Parameter.
@neilramsay
fix: CodePipeline Event Optional User Parameters
3fbfc4e 
The AWS CodePipeline Lambda Action does not require UserParameters
to be set, and therefore will be missing from the `action_configuration`.
https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-Lambda.html

This commit makes user_parameters optional (i.e. return None if the
key isn't present).
@neilramsay neilramsay requested a review from a team as a code owner April 12, 2023 04:21
@neilramsay neilramsay requested review from leandrodamascena and removed request for a team April 12, 2023 04:21
@boring-cyborg boring-cyborg Bot added the tests label Apr 12, 2023
@boring-cyborg

boring-cyborg Bot commented Apr 12, 2023

Copy link
Copy Markdown

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our AWS Lambda Powertools Discord: Invite link

@pull-request-size pull-request-size Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 12, 2023
@rubenfonseca

rubenfonseca commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Thank you so much for your contribution @neilramsay! As stated in our CONTRIBUTING guide all pull requests require an Issue first.

We'll try to create one for you as soon as possible.

@rubenfonseca rubenfonseca added do-not-merge need-issue PRs that are missing related issues labels Apr 12, 2023
@neilramsay

neilramsay commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

@rubenfonseca I'll create issue.

@neilramsay neilramsay mentioned this pull request Apr 12, 2023
Closed
@leandrodamascena leandrodamascena linked an issue Apr 12, 2023 that may be closed by this pull request
Bug: CodePipeline Event Data Class incorrectly handles optional User Configuration #2117
Closed
@leandrodamascena leandrodamascena removed do-not-merge need-issue PRs that are missing related issues labels Apr 12, 2023
@github-actions github-actions Bot added the bug Something isn't working label Apr 12, 2023
@github-actions

github-actions Bot commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions Bot added do-not-merge need-issue PRs that are missing related issues labels Apr 12, 2023
@leandrodamascena leandrodamascena removed do-not-merge need-issue PRs that are missing related issues labels Apr 12, 2023
@leandrodamascena

leandrodamascena commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@rubenfonseca I'll create issue.

Thank you for creating the issue @neilramsay! I'll review this PR till tomorrow.

@pull-request-size pull-request-size Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2023
@leandrodamascena leandrodamascena changed the title fix: CodePipeline Event Optional User Parameters fix(event_sources): add encryption_key field and change user parameters field Apr 12, 2023
leandrodamascena
leandrodamascena approved these changes Apr 12, 2023
View reviewed changes

@leandrodamascena leandrodamascena left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR looks good to me. Do you have any considerations before we merge it @neilramsay?

Comment thread aws_lambda_powertools/utilities/data_classes/code_pipeline_job_event.py
Comment on lines +100 to +109
class CodePipelineEncryptionKey(DictWrapper):
@property
def get_id(self) -> str:
return self["id"]

@property
def get_type(self) -> str:
return self["type"]


@leandrodamascena leandrodamascena Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @neilramsay! Looking at the documentation, I see that the encryptionKey field was missing. When you configure a codepipeline with a customer-managed KMS key, the codepipeline includes this field in the Lambda Event.

@leandrodamascena leandrodamascena changed the title fix(event_sources): add encryption_key field and change user parameters field fix(event_sources): Update event source to include optional encryption_key field and make user_parameters field optional Apr 12, 2023
@leandrodamascena leandrodamascena changed the title fix(event_sources): Update event source to include optional encryption_key field and make user_parameters field optional fix(event_sources): Update CodePipeline event source to include optional encryption_key field and make user_parameters field optional Apr 12, 2023
@neilramsay

neilramsay commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

@leandrodamascena nothing extra to add to this PR prior to merge.
If I find anything else, I'll raise a new issue+PR.

@leandrodamascena leandrodamascena merged commit 3e313bb into aws-powertools:develop Apr 12, 2023
@boring-cyborg

boring-cyborg Bot commented Apr 12, 2023

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@neilramsay neilramsay deleted the fix/codepipeline_event_optional_user_params branch May 1, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrodamascena leandrodamascena leandrodamascena approved these changes

Assignees

No one assigned

Labels

bug Something isn't working size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: CodePipeline Event Data Class incorrectly handles optional User Configuration

3 participants

@neilramsay @rubenfonseca @leandrodamascena