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

MIDRC-901 Add S3 endpoint #30

Merged
merged 17 commits into from
Dec 13, 2024
Merged

MIDRC-901 Add S3 endpoint #30

merged 17 commits into from
Dec 13, 2024

Conversation

paulineribeyre
Copy link
Collaborator

@paulineribeyre paulineribeyre commented Dec 12, 2024

Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/MIDRC-901

New Features

  • Add the /s3 endpoint, which receives incoming S3 requests, re-signs them with internal credentials, and forwards them to AWS S3

Improvements

  • Always log errors before returning them to the user

Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

be generated properly. This ensures unique operation IDs are generated for all routes.
"""
existing_routes = set()
for route in app.routes:

Choose a reason for hiding this comment

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

this section seems suspicious, although I think I see what you're doing.
making existing_routes a set is a good start.
we can filter out non api routes via

Suggested change
for route in app.routes:
api_routes_only = list(filter(lambda r: isinstance(r, APIRoute), app.routes)

then, you could do a for index, route in api_routes_only sort of thing.
the index is going to be unique for each route, so I don't think you'd need the while loop.
e.g.

Suggested change
for route in app.routes:
for index, route in enumerate(api_routes_only):
route.op_id = f"{route.name}_{index}"
existing_routes.add(route.operating_id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the goal is to avoid duplicate route IDs by calling them get_status, get_status_2, get_status_3 etc. Not adding a digit to all routes, although of course that would make them unique too 🙂

Copy link

@AlbertSnows AlbertSnows left a comment

Choose a reason for hiding this comment

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

tentative approval. I am not that familiar with s3 stuff, so I would suggest seeking a few more eyes before merging.

logger.error(f"Error from AWS: {response.status_code} {response.text}")

# return the response from AWS S3
if "Content-Type" in response.headers:

Choose a reason for hiding this comment

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

Do we only want to return response if the content-type is set? Probably true, but just curious 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We still return the response if the content-type isn't set, but in that case we don't include media_type=response.headers["Content-Type"] 👍

@jawadqur
Copy link

Apart from that one question this is very cool. This will be great!

@paulineribeyre paulineribeyre merged commit ec11731 into master Dec 13, 2024
8 checks passed
@paulineribeyre paulineribeyre deleted the feat/s3-api branch December 13, 2024 19:44
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