-
Notifications
You must be signed in to change notification settings - Fork 81
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
Pin alembic version to 'alembic==1.9.4' #354
Pin alembic version to 'alembic==1.9.4' #354
Conversation
I tested the fix locally but I am also testing the fix in an actual deployment. Tomorrow I will post here if the Pipeline completed successfully |
@@ -76,12 +76,12 @@ assume-role: | |||
rm .assume_role_json | |||
|
|||
drop-tables: upgrade-pip install-backend | |||
pip install alembic | |||
pip install 'alembic==1.9.4' |
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.
Just a heads up - 1.10.1
was released just a couple of days ago.
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.
Exactly, on March 6th they released 1.10.1 (data.all pipeline was working fine on March 5th) which included some regression fix that seems related to our issue.
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.
Hi @kukushking I have tested it in a deployment and the migration completes successfully (both in the validate migration step and in the actual migration).
What I was thinking is that instead of pinning the version directly in the CodeBuild command, we could add alembic as a backend package in backend/requirements.txt
so that it is treated as any other package. The only downside that I see is that alembic is not a strictly requirement of the backend compute resources. What is your opinion?
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 it's all right, no strong opinion on this one. As long as there is a comment specifying why it's there. Also considering migration script might be called outside CB.
…iadne 0.13 -> 0.17, fastapi 0.78 -> 0.92 (#379) ### Feature or Bugfix - Bugfix ### Detail - Upgrade starlette version: vulnerability found in starlette <0.25 (https://security.snyk.io/vuln/SNYK-PYTHON-STARLETTE-3319937). It does not affect data.all as we do not use `python-multipart` but nevertheless it is better to be in a non-vulnerable version. - Upgrade sqlalchemy version: the vulnerability is not stopping the CICD pipeline, but by upgrading we are able to use the latest version of alembic and we can revert the pinning of the version which happened in #354 - Upgrade ariadne to version 0.17.0: needed to support starlette 0.25.0 Higher version of ariadne==0.18.0 removes `PLAYGROUND_HTML` constant that we use in testing (Check [docs](https://ariadnegraphql.org/docs/0.17/constants-reference)) - Upgrade fastapi version to 0.92.0: needed to support starlette 0.25.0 (Version that supports this particular version of starlette, [docs](https://fastapi.tiangolo.com/release-notes/#0920)) ### Relates - #378 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Feature or Bugfix
Detail
Relates
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.