Skip to content

Import error read endpoints and schema #9217

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
mik-laj merged 1 commit into from
Jun 16, 2020
Merged

Import error read endpoints and schema #9217

mik-laj merged 1 commit into from
Jun 16, 2020

Conversation

@OmairK
Copy link
Contributor

@OmairK OmairK commented Jun 10, 2020 •
edited
Loading

Closes #8130

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Jun 10, 2020
@OmairK OmairK changed the title [WIP] import error endpoints [WIP] import error read endpoints and schema Jun 10, 2020
mik-laj
mik-laj reviewed Jun 12, 2020
View reviewed changes
airflow/api_connexion/openapi/v1.yaml Outdated
Copy link
Member

@mik-laj mik-laj Jun 12, 2020

Choose a reason for hiding this comment

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

😻

mik-laj
mik-laj reviewed Jun 12, 2020
View reviewed changes
mik-laj
mik-laj reviewed Jun 12, 2020
View reviewed changes
Copy link
Member

@mik-laj mik-laj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We are very close to the end. The main problem is static checking. As long as the change is not green (excluding quarantine tests), I cannot merge it.
Here is a guide about static checks: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst
Can you do a rebase also? We have a merge conflicts.

@OmairK OmairK force-pushed the import-errors-8130 branch from 3659283 to 4b13809 Compare June 12, 2020 18:24
@OmairK OmairK changed the title [WIP] import error read endpoints and schema Import error read endpoints and schema Jun 12, 2020
@OmairK OmairK force-pushed the import-errors-8130 branch from 4b13809 to 287932c Compare June 12, 2020 19:09
@mik-laj
Copy link
Member

mik-laj commented Jun 14, 2020 

tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorEndpoint::test_response_200
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpoint::test_get_import_errors
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_0__api_v1_importErrors_limit_1
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_1__api_v1_importErrors_limit_110
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_2__api_v1_importErrors_offset_1
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_3__api_v1_importErrors_offset_3
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_4__api_v1_importErrors_offset_3_limit_3

Some API-related tests were unsuccessful. Could you look at it?

@OmairK OmairK force-pushed the import-errors-8130 branch from 287932c to 3ed24b3 Compare June 15, 2020 07:24
@OmairK
Copy link
Contributor Author

OmairK commented Jun 15, 2020 

tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorEndpoint::test_response_200
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpoint::test_get_import_errors
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_0__api_v1_importErrors_limit_1
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_1__api_v1_importErrors_limit_110
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_2__api_v1_importErrors_offset_1
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_3__api_v1_importErrors_offset_3
FAILED tests/api_connexion/endpoints/test_import_error_endpoint.py::TestGetImportErrorsEndpointPagination::test_limit_and_offset_4__api_v1_importErrors_offset_3_limit_3

Some API-related tests were unsuccessful. Could you look at it?

That's weird, these tests are running fine in my breeze environment(both python3.6 python 3.7)

@OmairK OmairK force-pushed the import-errors-8130 branch from 3ed24b3 to 8260bac Compare June 15, 2020 08:25
@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020 •
edited
Loading

@OmairK Okay. I looked at what the problem was. The problem is that clear_db_import_errors function doesn't reset the sequence. Each newly added element has a new identifier. However, if the object was created in another test, then the numbering continues in your test. If you run one test several times, only the first run will be correct, but all others will fail. I have prepared a patch that normalizes identifiers in responses.
https://github.com/apache/airflow/pull/9217/commits
This should help. What do you think about this?

@OmairK
Copy link
Contributor Author

OmairK commented Jun 15, 2020

@OmairK Okay. I looked at what the problem was. The problem is that clear_db_import_errors function doesn't reset the sequence. Each newly added element has a new identifier. However, if the object was created in another test, then the numbering continues in your test. If you run one test several times, only the first run will be correct, but all others will fail. I have prepared a patch that normalizes identifiers in responses.
https://github.com/apache/airflow/pull/9217/commits
This should help. What do you think about this?

Ah it was the sequence all along, I get it thank you so much for the help. A couple of quick questions

  1. I was running these tests locally by ./breeze test-target tests/api_connexion/ these tests should have failed there too unless the sequence was automatically resetting after every test, which doesn't make sense. Is there some major difference between how breeze and github action CI handles the db state between tests ?
  2. If yes, how do you reenact your airflow github actions locally ? (This would help me a lot in the future)

P.S.: +1 for the commit message, it perfectly sums up the situation :)

@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020

  1. I will reproduce this problem locally. I launched Breeze with Postgres and Python 3.8 and have run the following command:
pytest tests/api_connexion/endpoints/test_import_error_endpoint.py

I suspect you use SQLite, which sometimes behaves differently

@OmairK OmairK force-pushed the import-errors-8130 branch 2 times, most recently from bf4bf97 to 1a6633c Compare June 15, 2020 13:40
@OmairK OmairK changed the title Import error read endpoints and schema [WIP]Import error read endpoints and schema Jun 15, 2020
@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020

Caa you do a rebase to the latest master?

Is this still WIP? There is still a label in the PR title.

@OmairK OmairK force-pushed the import-errors-8130 branch from 1a6633c to c7ec872 Compare June 15, 2020 19:44
@OmairK OmairK changed the title [WIP]Import error read endpoints and schema Import error read endpoints and schema Jun 15, 2020
@OmairK
Copy link
Contributor Author

OmairK commented Jun 15, 2020

Is this still WIP? There is still a label in the PR title.

No, I had 2 PRs to be merged at the same time so I made this PR WIP just to keep track.

@OmairK OmairK force-pushed the import-errors-8130 branch from c7ec872 to f37c3f5 Compare June 16, 2020 05:57
@mik-laj mik-laj merged commit 47bddf7 into apache:master Jun 16, 2020
@OmairK OmairK deleted the import-errors-8130 branch June 16, 2020 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mik-laj mik-laj mik-laj approved these changes

Assignees

No one assigned

Labels

area:API Airflow's REST/HTTP API area:webserver Webserver related Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

API Endpoints - Read-only - Import errors

2 participants

@OmairK @mik-laj