-
Notifications
You must be signed in to change notification settings - Fork 3
[IT-4485] Refactor to prep for adding new endpoint #52
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
[IT-4485] Refactor to prep for adding new endpoint #52
Conversation
Refactor the lambda into multiple submodules in preparation of adding a new API endpoint for getting the current balances. Also rename 'mips' to 'mip' to accurately reflect the name of the upstream service.
| return out_chart | ||
|
|
||
|
|
||
| def limit_chart(params, chart_dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically long lists are handled by pagination, i.e. the client can limit the (page) size but then it can also request later page(s) to get all the content. What's the implication of having truncation without a way to get the rest of the content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I hadn't realized that this is basically half of a pagination implementation. The use case this is supporting is truncating the chart to the first 25 entries for configuring Service Catalog tag options: https://github.com/Sage-Bionetworks-IT/organizations-infra/blob/master/sceptre/scipool/config/prod/sc-tag-options.yaml#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for providing more context. So it's critical here that the items in the chart are prioritized, so that if a client can only handle n items, it can say, "give me the n most important ones." (Maybe it's more analogous to string truncation than to pagination.) With this understanding I think the code is fine, but perhaps you can just add a comment like, "limit the response to the chosen number of high priority items."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the lambda codebase by breaking apart a monolithic mips_api module into multiple focused submodules in preparation for adding a new API endpoint. The refactoring improves code organization and maintainability while also correcting the service name from 'mips' to 'mip' throughout the codebase.
Key changes include:
- Splitting the single
mips_api/__init__.pyfile into separate modules (chart.py,upstream.py,ssm.py,s3.py,util.py) - Renaming the package from
mips_apitomip_apito accurately reflect the upstream service name - Adding S3 lifecycle configuration for cleaning up old object versions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_handler.py | Updates all test imports and function calls to use the new mip_api module structure |
| template.yaml | Changes Lambda handler reference and adds S3 lifecycle policy for version cleanup |
| mips_api/init.py | Removes the original monolithic module (543 lines deleted) |
| mip_api/util.py | New utility module containing helper functions for parameter parsing and validation |
| mip_api/upstream.py | New module handling all upstream API interactions with MIP Cloud |
| mip_api/ssm.py | New module for AWS Systems Manager parameter retrieval |
| mip_api/s3.py | New module for S3 cache operations |
| mip_api/chart.py | New module for chart of accounts processing and business logic |
| mip_api/init.py | New main module containing only the Lambda handler function |
| .github/workflows/test.yaml | Removes pre-commit job from the workflow |
| .coveragerc | Updates source path to use new mip_api module name |
Comments suppressed due to low confidence (1)
mip_api/util.py:50
- [nitpick] The variable 'number' is not descriptive. Consider renaming to 'limit_value' or 'parsed_limit' to better indicate its purpose.
number = int(params["limit"])
4fcf125 to
49bb9b3
Compare
brucehoff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one follow up suggestion (documentation) but I don't think that should hold up approving.
49bb9b3 to
51c5912
Compare
0b660f4
into
Sage-Bionetworks-IT:master
Refactor the lambda into multiple submodules in preparation of adding a new API endpoint for getting the current balances. The new API endpoint will use the chart of accounts for a different segment (not "Program") and collate with data from an additional upstream API endpoint.
Also rename 'mips' to 'mip' to accurately reflect the name of the upstream service.