Changed user-fi model to allow nullable/empty state field #210
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Looks good. In the long run, I'd be interested in coming up with some other solution for these edge cases where we don't have to allow null on state, but for now 👍.
Just the one question about the greenlet change package and whether it should be part of this PR or not.
pyproject.toml
Outdated
| @@ -15,6 +15,7 @@ asyncpg = "^0.29.0" | |||
| alembic = "^1.13.2" | |||
| regtech-api-commons = {git = "https://github.com/cfpb/regtech-api-commons.git"} | |||
| regtech-regex = {git = "https://github.com/cfpb/regtech-regex.git"} | |||
| greenlet = "^3.0.3" | |||
There was a problem hiding this comment.
Was this to fix the Mac chip issue? And was this coming in as a dependency of something else before?
There was a problem hiding this comment.
Yes, sorry for the delayed response. This was coming with SQLAlchemy but poetry seems to have an issue finding the M-series chips distros for greenlet, so the suggestions I found online was to simply explicitly define the dependency versus relying on poetry to install the dependency. Didn't come across this issue until the new Mac. And only is an issue when trying to run things within poetry.
One solution for images I found is adding --platform=linux/amd64 to the Dockerfile FROM statement so it forces a particular architecture in the image, alleviating potential issues between where the image is built and what it's run on (found that messing with lambdas). Wouldn't help the reason for this change, but thinking of adding stories to all our repos to make that a thing across the board. Thoughts?
There was a problem hiding this comment.
Interesting. At first I was surprised to see greenlet in there. I thought it was more of a pre-asyncio way of doing async in Python. Looking at Using events with the asyncio extension (you gotta scroll down to asyncio and events, two opposites), it looks like internally it handles async via greenlet, and then does a asyncio wrapper API that dev interact with. Makes sense. SQLAlchemy has been around way longer than asyncio, so that's probably a way to add support without changing the guts of SQLAlchemy. Anyway, I get why it's there now. 😄
Looking at I’m getting an error about greenlet not being installed when I try to use asyncio, it sounds like we can fix this by using the sqlalchemy[asyncio] package. If that works, might be better to go that route in case there are other asyncio-related bits added to that package.
And yeah, I've read about building containers images for different architectures, but I've never had to do it. So, you're ahead of me on that front. Probably worth a chat to sync up on that and what you've learned. As you say, don't have to solve that for this PR.
There was a problem hiding this comment.
Ok found the way to do the equivalent in of sqlalchemy[asyncio] in pyproject.toml, versus explicitly pulling in greenlet.
Closes #209