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

Added API endpoint for file manifest #76

Merged
merged 2 commits into from
Jul 16, 2022
Merged

Conversation

aequis
Copy link
Contributor

@aequis aequis commented Oct 24, 2020

I saw the issue at https://github.com/eepMoody/open5e-api/issues/57 and decided to take a stab at it. I'm open to any feedback or suggestions for improvements.

I've added a function that calculates the sha256 hash of a given file. The hash function is called inside the populatedb management command whenever a new data file is opened. The resulting hash is stored with the corresponding file path in the database in its own model, Manifest.

Let me know if there's a better place to calculate the file hashes, or if you have better suggestions for the model name or anything else.

I've also added a DRF read-only ViewSet (and registered it with the router) for the Manifest model. This allows anybody to grab a list of all the data files and their file hashes and use it to check if any data files have changed since the last time they checked the manifest endpoint.

Also, the populatedb.py is a bit verbose with quite a bit of unnecessary duplication, especially with my additions. My next step would be to reduce all of that as much as possible, assuming you're okay with it.

@eepMoody
Copy link
Collaborator

eepMoody commented Nov 2, 2020

@aequis I just wanted to check in, it looks like there's a syntax error that's causing the populatedb.py script to fail. I'm seeing it on my machine as well, but I haven't quite traced it down. You seeing it locally as well if you flush the DB and do a full rebuild?

@eepMoody
Copy link
Collaborator

eepMoody commented Nov 2, 2020

Once that's resolved this looks great, and like an awesome addition. I'll get it merged in as soon as it passes!

@aequis
Copy link
Contributor Author

aequis commented Nov 2, 2020

@aequis I just wanted to check in, it looks like there's a syntax error that's causing the populatedb.py script to fail. I'm seeing it on my machine as well, but I haven't quite traced it down. You seeing it locally as well if you flush the DB and do a full rebuild?

Huh, odd. It's working fine here on Windows if I run it locally. I'll have to do some testing when I have time. Thanks for letting me know. Do you have an error message?

@eepMoody
Copy link
Collaborator

eepMoody commented Jul 5, 2022

@aequis Hey I know it's been like a year, but I finally fixed the deploy issues (caused by a non-obvious change to dockerhub, insert facepalm here) that were keeping me from getting useful error messages and ensuring that this was a real issue.

The issue is in populatedb.py:13, where you're leveraging a python 3.8 feature (the := operator). This project is built on python 3.7. I suspect you're running 3.8+ on your local system.

I've been looking into trying to update, but it's risky operation that risks bringing the server down if it doesn't go smooth.

I'm not quite sure what the intent of this line is, so I can't figure out how to rewrite it without the walrus operator. :(

@eepMoody eepMoody merged commit d8d23f7 into open5e:master Jul 16, 2022
@Gredelston Gredelston mentioned this pull request Mar 11, 2023
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