Skip to content
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

Fix issue with airflow-2-6-0rc1 #1899

Merged
merged 18 commits into from
May 3, 2023
Merged

Fix issue with airflow-2-6-0rc1 #1899

merged 18 commits into from
May 3, 2023

Conversation

pankajastro
Copy link
Contributor

@pankajastro pankajastro commented Apr 16, 2023

Description

What is the current behavior?

Tested with Airflow 2.6rc and found there are a couple of issues related

  1. serialization/deserialization - Table and Column
  2. Test failing airflow.jobs.scheduler_job.SchedulerJob and airflow.jobs.backfill_job.BackfillJob Classes removed.

Closes: #1904

What is the new behavior?

Added fixes for the above issues.

Does this introduce a breaking change?

Nope.

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@pankajastro pankajastro force-pushed the airflow-rc-2-6-0 branch 2 times, most recently from 6e6cb73 to fc5920e Compare April 16, 2023 16:40
@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Patch coverage: 76.19% and project coverage change: -1.35 ⚠️

Comparison is base (50329e5) 86.13% compared to head (e9a112f) 84.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
- Coverage   86.13%   84.78%   -1.35%     
==========================================
  Files         127      104      -23     
  Lines        6837     5950     -887     
  Branches      675      677       +2     
==========================================
- Hits         5889     5045     -844     
+ Misses        794      761      -33     
+ Partials      154      144      -10     
Flag Coverage Δ
PythonSDK 92.35% <76.19%> (+0.55%) ⬆️
SQL-CLI ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python-sdk/src/astro/sql/operators/cleanup.py 91.59% <66.66%> (-1.45%) ⬇️
python-sdk/src/astro/table.py 97.14% <77.77%> (-1.83%) ⬇️
python-sdk/src/astro/constants.py 94.91% <83.33%> (-1.39%) ⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@dimberman dimberman left a comment

Choose a reason for hiding this comment

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

Overall LGTM minus a few nits. Can you please add some more description as to why there is Serialization issues with airflow 1.6? Also I think you should change the title to something like "Fix serialization issues related to Airflow 2.6"

python-sdk/src/astro/sql/operators/cleanup.py Show resolved Hide resolved
python-sdk/src/astro/table.py Outdated Show resolved Hide resolved
@utkarsharma2 utkarsharma2 changed the title Run tests on airflow-2-6-0b1 Fix issue with airflow-2-6-0b1 Apr 21, 2023
@utkarsharma2 utkarsharma2 changed the title Fix issue with airflow-2-6-0b1 Fix issue with airflow-2-6-0rc1 Apr 24, 2023
@pankajastro pankajastro marked this pull request as ready for review April 24, 2023 12:11
Copy link
Contributor Author

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

Thank you @utkarsharma2 for looking into it. LGTM.

@pankajkoti pankajkoti removed their request for review May 3, 2023 04:55
@tatiana tatiana force-pushed the airflow-rc-2-6-0 branch from d771139 to 6fd5ba7 Compare May 3, 2023 12:56
@tatiana tatiana force-pushed the airflow-rc-2-6-0 branch from 6fd5ba7 to e9a112f Compare May 3, 2023 13:16
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

This is a blocker for the 1.6 release, which is expected by customers urgently. For this reason, I'm approving and merging the PR before we meet our desired code coverage (currently, we only have 76.19% of diff hit, while our target for diff coverage is 86.13%).

@tatiana tatiana merged commit 1ea5c0c into main May 3, 2023
@tatiana tatiana deleted the airflow-rc-2-6-0 branch May 3, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AstroSDK/Airflow 2.6.0 Compatibility issues
4 participants