-
-
Notifications
You must be signed in to change notification settings - Fork 253
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
Render if_not_exists
option for CreateTableOp, CreateIndexOp, DropTableOp and DropIndexOp
#1446
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.
sorry I kinda lost this PR.
configurable via AutogenContext
It may make sense. @zzzeek what do you think about adding an option to control this?
It's missing a changelog. see example in https://github.com/sqlalchemy/alembic/tree/main/docs/build/unreleased
Feel free to add something like "pull request courtesy of "
7defc52
to
7ddeaab
Compare
Hi @CaselIT, thanks for the review. I rebased upon main branch & added a changelog entry. Feel free to tell me how in any way I can now help this pull request move forward. |
Let's wait on mike's opinion on the flag(s) in autogenerate |
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.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 7ddeaab of this pull request into gerrit so we can run tests and reviews and stuff
New Gerrit review created for change 7ddeaab: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5454 |
7ddeaab
to
cb57978
Compare
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, this is sqla-tester and I see you've pinged me for review. However, user lachaib is not authorized to initiate CI jobs. Please wait for a project member to do this!
hi! sorry didnt see this. Turns out we are also getting ready to merge the same option for tables in #1521. So I want to get #1521 merged and then we can update this to support autogen for both index and table, hows that? I understand this got ignored for 6 months so if you aren't around I'll just pull this in and finish it up. |
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.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision cb57978 of this pull request into gerrit so we can run tests and reviews and stuff
Patchset cb57978 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5454 |
Ok ping me when the other PR is merged and I'll take care. |
Michael Bayer (zzzeek) wrote: OK this is ready for adding if_exists / if_not_exists rendering support for create_table and drop_table also View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5454 |
I've seen the PR merged, gonna take care of this over the weekend |
Thanks! |
cb57978
to
0e7deeb
Compare
0e7deeb
to
90c9735
Compare
if_not_exists
option for CreateIndexOp and DropIndexOpif_not_exists
option for CreateTableOp, CreateIndexOp, DropTableOp and DropIndexOp
sorry I get no notification that anything changed here, will run it through now |
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.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 90c9735 of this pull request into gerrit so we can run tests and reviews and stuff
Patchset 90c9735 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5454 |
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5454 has been merged. Congratulations! :) |
alembic 1.13.3 is released now with this change |
Related: #151
Description
Without opinion on whether or not the option
if_not_exists
orif_exists
should be set by default (or configurable via AutogenContext), I think a follow-up on#151 should be that
if_not_exists
orif_exists
attributes should be that they are rendered during autogen steps.How I intend to use it: specify the attribute during rewrites of the pipeline
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>
in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>
in the commit messageHave a nice day!