Skip to content

[AIRFLOW-5793] add test to detect multiple alembic revision heads #6449

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.

Sign up for GitHub

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

Jump to bottom
Merged
Fokko merged 1 commit into from
Oct 28, 2019
Merged

[AIRFLOW-5793] add test to detect multiple alembic revision heads #6449

Fokko merged 1 commit into from
Oct 28, 2019

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Oct 27, 2019

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Depending on the timing of merges with migrations, we can end up with two revision heads that need to be merged.

This adds a test to detect when multiple heads are present.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@dstandish dstandish mentioned this pull request Oct 27, 2019
Merged
5 tasks
@dstandish
Copy link
Contributor Author

dstandish commented Oct 27, 2019

@Fokko was unsure about exactly right naming for test file and location. e.g. could have called it test_migrations instead of test_alembic. do lemme know if you think something else is better and i'll update.

Fokko
Fokko reviewed Oct 27, 2019
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, one comment

tests/test_alembic.py Outdated
Copy link
Contributor

@Fokko Fokko Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this test to https://github.com/apache/airflow/blob/master/tests/utils/test_db.py

This file contains a closely related test, to check if the schema of the database, and the schema in the python files are in sync.

Copy link
Contributor

@Fokko Fokko Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add a small comment on when we could have two heads, and how to resolve it :-)

Copy link
Member

@feluelle feluelle Oct 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better to have in the PR template? (and also in the test)

Copy link
Member

@potiuk potiuk Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR template is already pretty bloated :) (I think we should somehow simplify this). I found it initially useful, but then it becomes more and more tiresome. Let's not make it more complex.

I think everything that is now in the template should be automatically verified by the checks in CI and commiters should not even think about checking it.

Copy link
Member

@potiuk potiuk Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Fokko -> better have less test files.

Copy link
Member

@feluelle feluelle Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR template is already pretty bloated :) (I think we should somehow simplify this). I found it initially useful, but then it becomes more and more tiresome. Let's not make it more complex.

Totally agree. Would be nice if we the template changes depending on the files that changed.

Copy link
Contributor

@Fokko Fokko Oct 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase onto master and move this test to the other file? :-)

Copy link
Contributor Author

@dstandish dstandish Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes will do very shortly was out for a bit

Copy link
Contributor Author

@dstandish dstandish Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved test. added small comment, just pointing user to alembic merge command.

@dstandish dstandish force-pushed the feature/test-for-multiple-alembic-revision-heads branch from b6a6b44 to 737ae44 Compare October 28, 2019 00:45
@codecov-io
Copy link

codecov-io commented Oct 28, 2019 •
edited
Loading

Codecov Report

Merging #6449 into master will decrease coverage by 0.3%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6449      +/-   ##
==========================================
- Coverage   83.99%   83.68%   -0.31%     
==========================================
  Files         627      627              
  Lines       36537    36537              
==========================================
- Hits        30690    30577     -113     
- Misses       5847     5960     +113
Impacted Files Coverage Δ
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/utils/dag_processing.py 58.15% <0%> (-0.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171e1bb...737ae44. Read the comment docs.

@Fokko Fokko merged commit 37a342d into apache:master Oct 28, 2019
@Fokko
Copy link
Contributor

Fokko commented Oct 28, 2019

Thanks @dstandish

1 similar comment
@kaxil
Copy link
Member

kaxil commented Oct 28, 2019

Thanks @dstandish

@dstandish dstandish deleted the feature/test-for-multiple-alembic-revision-heads branch January 1, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@feluelle feluelle feluelle left review comments

@potiuk potiuk potiuk approved these changes

@Fokko Fokko Fokko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dstandish @codecov-io @Fokko @kaxil @potiuk @feluelle