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

DE-101: Re-platform application to Python #11

Merged
merged 20 commits into from
Jul 23, 2024
Merged

Conversation

fatimarahman
Copy link
Contributor

@fatimarahman fatimarahman commented Jul 17, 2024

Changes made

  • Eliminated programmatic cache to store schemas. Schema retrieval is relatively fast and our scale of work is relatively small at this point -- we don't have much of a need for it
  • Eliminated schema handling. Instead, I've decided to retool python-utils' existing functionality to pull Avro schemas as well as decode records. This is more space efficient plus I wanted to leverage existing code in our org-wide utils
  • The obvious -- everything is in Python now!

Related story
DE-101

Initially assigning just so you can take a look at the code! Won't merge in until python utils is merged

@fatimarahman
Copy link
Contributor Author

fatimarahman commented Jul 17, 2024

Should I update the .travis.yml for Python? Wasn't sure how to handle this. Should I also update the provisioning as well?

@fatimarahman
Copy link
Contributor Author

Also @aaronfriedman6 I can't update the settings for this repo so that we can perform Python unit tests in the checks rip

@aaronfriedman6
Copy link
Contributor

Should I update the .travis.yml for Python? Wasn't sure how to handle this. Should I also update the provisioning as well?

You can remove everything related to Travis -- we don't use it anymore. Instead, we use GitHub actions to handle deployments.

Also @aaronfriedman6 I can't update the settings for this repo so that we can perform Python unit tests in the checks rip

What settings need to be changed?

@fatimarahman
Copy link
Contributor Author

Nvm I think I figured it out! In the past I've had to hook up running tests automatically through the Github settings

Copy link
Contributor

@aaronfriedman6 aaronfriedman6 left a comment

Choose a reason for hiding this comment

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

Looks great! Some small things but nothing major. I haven't reviewed the tests yet but figured I'd wait in case any of my comments required changing the tests too

.python-version Outdated Show resolved Hide resolved
.github/workflows/run-unit-tests.yml Outdated Show resolved Hide resolved
config/production.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file used by the python lambda package you mentioned? Also remove SCHEMA_NAME var below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be used for sam local or the python lambda local usage! This is mainly a holdover from the NodeJS implementation, since we have plenty of sample events (in the sample folder) to use and verify when running local changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

So does sam local or the python lambda package actually run the QA Lambda function or does it just mimic it? Like when you run it could I go on AWS and see the logs from the run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sam local mimics the Lambda function locally and stores the logs locally. So this will not be visible on AWS. When you run the command, the output shows up on the terminal.

lambda_function.py Show resolved Hide resolved
lambda_function.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
record_processor.py Show resolved Hide resolved
Comment on lines 46 to 49
# After decoding, convert to base64. More often than not,
# the original data is either encoded or not in base64
to_bytes = data.encode("utf-8")
return (base64.b64encode(to_bytes)).decode("utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I don't totally get this -- could explain a little more what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little convoluted but we need to encode the decoded data to base64, and base64.b64encode() requires a bytes-like input. In addition, the .decode("utf-8") removes the byte wrapper around the decoded output. I.e. transforms b'Y2N8TGlicmFyeSBcfCBDICEsPXxXZWR8MDE6MDB8MjM6MDB8MjAyMy0wMy0wMwo=' to Y2N8TGlicmFyeSBcfCBDICEsPXxXZWR8MDE6MDB8MjM6MDB8MjAyMy0wMy0wMwo=.

Copy link
Contributor

Choose a reason for hiding this comment

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

So data is a regular string, then you convert it to UTF-8 bytes, then you convert those UTF-8 bytes to hex bytes, then you convert those hex bytes to a hex string? Could you add a comment saying that data is a regular string and we want to output that same string in hex, which requires using bytes as an intermediate type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly. I'll add the comment in rn!

Copy link
Contributor

@aaronfriedman6 aaronfriedman6 left a comment

Choose a reason for hiding this comment

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

Looks great -- I'm ready to see it in action!

config/devel.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

So does sam local or the python lambda package actually run the QA Lambda function or does it just mimic it? Like when you run it could I go on AWS and see the logs from the run?

devel_requirements.txt Outdated Show resolved Hide resolved
lambda_function.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
record_processor.py Outdated Show resolved Hide resolved
tests/test_record_processor.py Outdated Show resolved Hide resolved
tests/test_record_processor.py Show resolved Hide resolved
tests/test_record_processor.py Outdated Show resolved Hide resolved
tests/test_lambda_function.py Outdated Show resolved Hide resolved
@fatimarahman
Copy link
Contributor Author

Assigning it to you one last time since I replied to some of your comments! Also, should I merge to main after this?

@aaronfriedman6
Copy link
Contributor

Looks good! Yep merge to main and we'll talk offline about how to deploy this

@fatimarahman fatimarahman merged commit b64d645 into main Jul 23, 2024
@fatimarahman fatimarahman deleted the de-101/python-rewrite branch July 23, 2024 17:29
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.

2 participants