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

Add google/latest endpoint #35

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Add google/latest endpoint #35

merged 1 commit into from
Jun 27, 2024

Conversation

F-X64
Copy link
Member

@F-X64 F-X64 commented Jun 26, 2024

Add google/latest
Add optional arch param
Add tests to validate function with or without supplying optional param

@F-X64 F-X64 requested a review from major June 26, 2024 09:26
print(arch)
latest_image = (
db.query(GoogleImage)
.filter(GoogleImage.arch.ilike(arch))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is interesting.
sqalchemy queries seem to be case sensitive by default (which is expected).
running a query like this will work:
"localhost:8000/google/latest?arch=ARM64"

but if I run this:
"localhost:8000/google/latest?arch=X86_64"
it will automatically be tranformed to "...x86_64" which will crash.
Used ilike comparison to mitigate.

Copy link
Member

Choose a reason for hiding this comment

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

That's an interesting variance. 🤔

Copy link
Member

@major major left a comment

Choose a reason for hiding this comment

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

A few questions here.

cid/crud.py Outdated
)
print(archs)
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to leave this print() here? 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope 🤦

cid/crud.py Outdated
return {"error": "No images found", "code": 404}
if latest_image is None:
print("No image found")
continue
Copy link
Member

Choose a reason for hiding this comment

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

Should we return a 404 here?

@F-X64 F-X64 force-pushed the add-google-latest-endpoint branch from 21d78d4 to e4d43c2 Compare June 27, 2024 06:25
@F-X64 F-X64 requested a review from major June 27, 2024 06:25
Copy link
Member

@major major left a comment

Choose a reason for hiding this comment

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

LGTM, but this one needs a rebase.

Add google/latest
Add optional arch param
Add tests to validate function with or without supplying opptional param
@F-X64 F-X64 force-pushed the add-google-latest-endpoint branch from e4d43c2 to 9d9d002 Compare June 27, 2024 12:35
@major major merged commit 70c5f5c into main Jun 27, 2024
3 checks passed
@major major deleted the add-google-latest-endpoint branch June 27, 2024 12:38
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