You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "syun64 (via GitHub)" <gi...@apache.org> on 2023/09/13 13:54:08 UTC
[GitHub] [airflow] syun64 opened a new pull request, #34344: Bug fix on clear number migration script
syun64 opened a new pull request, #34344:
URL: https://github.com/apache/airflow/pull/34344
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jens-scheffler-bosch commented on pull request #34344: Bug fix on clear number migration script
Posted by "jens-scheffler-bosch (via GitHub)" <gi...@apache.org>.
jens-scheffler-bosch commented on PR #34344:
URL: https://github.com/apache/airflow/pull/34344#issuecomment-1718201128
Can you add a description what this PR is for? Any relation to a bug or feature?
Otherwise if you experiment-around, maybe mark the PR as DRAFT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala merged pull request #34344: Bug fix on clear_number migration script to properly set the default value
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #34344:
URL: https://github.com/apache/airflow/pull/34344
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bug fix on clear_number migration script to properly set the default value [airflow]
Posted by "boctorj (via GitHub)" <gi...@apache.org>.
boctorj commented on code in PR #34344:
URL: https://github.com/apache/airflow/pull/34344#discussion_r1581498733
##########
airflow/migrations/versions/0129_2_8_0_add_clear_number_to_dag_run.py:
##########
@@ -38,15 +37,22 @@
def upgrade():
"""Apply add cleared column to dagrun"""
- with op.batch_alter_table("dag_run") as batch_op:
- batch_op.add_column(
- sa.Column(
- "clear_number",
- sa.Integer,
- default=0,
- nullable=False,
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ with op.batch_alter_table("dag_run") as batch_op:
+ batch_op.add_column(sa.Column("clear_number", sa.Integer, default=0))
+ batch_op.alter_column("clear_number", existing_type=sa.Integer, nullable=False)
Review Comment:
Thanks for fixing the issue but this doesn't fixed mssql. Is there a reason why you forked the code to support this?
alter_column can take a 'server_default' value as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] syun64 commented on pull request #34344: Bug fix on clear_number migration script to properly set the default value
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #34344:
URL: https://github.com/apache/airflow/pull/34344#issuecomment-1728073454
@phanikumv I've resolved the merge conflict, and tests were passing but 'Mergeable' check was stuck 'In Progress' - triggered another CI run by updating the branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] syun64 commented on pull request #34344: [DRAFT] Bug fix on clear number migration script
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #34344:
URL: https://github.com/apache/airflow/pull/34344#issuecomment-1718202590
> Can you add a description what this PR is for? Any relation to a bug or feature? Otherwise if you experiment-around, maybe mark the PR as DRAFT?
Sorry about that @jens-scheffler-bosch ! made the requested change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] syun64 commented on pull request #34344: Bug fix on clear_number migration script to properly set the default value
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #34344:
URL: https://github.com/apache/airflow/pull/34344#issuecomment-1718489269
tagging @ephraimbuddy and @abhishekbhakat for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bug fix on clear_number migration script to properly set the default value [airflow]
Posted by "abhishekbhakat (via GitHub)" <gi...@apache.org>.
abhishekbhakat commented on code in PR #34344:
URL: https://github.com/apache/airflow/pull/34344#discussion_r1581731440
##########
airflow/migrations/versions/0129_2_8_0_add_clear_number_to_dag_run.py:
##########
@@ -38,15 +37,22 @@
def upgrade():
"""Apply add cleared column to dagrun"""
- with op.batch_alter_table("dag_run") as batch_op:
- batch_op.add_column(
- sa.Column(
- "clear_number",
- sa.Integer,
- default=0,
- nullable=False,
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ with op.batch_alter_table("dag_run") as batch_op:
+ batch_op.add_column(sa.Column("clear_number", sa.Integer, default=0))
+ batch_op.alter_column("clear_number", existing_type=sa.Integer, nullable=False)
Review Comment:
Discussed here: https://github.com/apache/airflow/pull/34335/files/ae9920a4be57acf30ee6a7923d3a1e4708a1c36d
Gist: _Issue was with downgrade._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org