-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Conversation
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.
Can we use a matrix here and test LTS and the newest python version?
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.
added a matrix in circle ci for make test and one in database.yml sqlite job for make build-db which are python version dependent moved the formatting and openapi spec generation to a separate step in circleci since they are not python version dependent
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.
Do we need this file? I'd like to support multiple Python versions.
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.
well there are a few benefits to having the interpreter version pinned, the pipeline uv sets up automatically + make install/setup (uv sync) auto installs the suitable python version
I think having a pinned python version for the project is fine since uv handles that anyways and to run it with diff versions locally the developer can either use the --python flag with uv or modify the version pin temporarily https://docs.astral.sh/uv/concepts/python-versions/#requesting-a-version
instead in the pipeline we can have the matrix which runs the project against diff version and the dev can later modify it locally we can make a note in the readme
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 think adding a default python version is probably a good idea, but I agree that we should test older versions. I think adding them to the testing matrix is a good approach.
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.
Why false?
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 moved all the dev settings to the -dev compose file and left this as barebones base compose file, the graphql console is not really a recommended setting to be enabled to be in prod/ is a local/dev based setting so figured overriding it in the dev compose file is a more suitable location for it
is there any particular reason for us to have it globally set to true I am not that familiar with graphql but shouldnt the console be enabled in a much more limited manner or does it not matter in our case?
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'm not as familiar with the docker and pg_dump commands in this YAML, but this seems separate from the UV shift. I'm not opposed, but could you describe what the changes here would do?
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.
Should we be concerned with the automated warnings from Github on this line?
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 think adding a default python version is probably a good idea, but I agree that we should test older versions. I think adding them to the testing matrix is a good approach.
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.
Less a question directed at you, but is there a reason we have two targets that do the same thing (setup and migrate)?
Uh oh!
There was an error while loading. Please reload this page.
Change description
First set of pr for migrating and adopting modern python tooling ecosystem, this pr attempts to migrate from pip to uv for package management as discussed in #1391 & #1521 it breaks down the changes @phalt initially made to smaller more iterative parts
cc: @phalt @Naramsim
AI coding assistance disclosure
Used a bit for reviewing the changes and coverage if I missed any transitive command update
Contributor check list