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

Add possibility to run DAGs from system tests and see DAGs logs #17868

Merged
merged 5 commits into from
Aug 31, 2021

Conversation

deedmitrij
Copy link
Contributor

  • Change 'schedule_interval' from 'None' to '@once'
  • Add 'setUp' and 'tearDown' methods for logs collecting

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@deedmitrij deedmitrij requested a review from turbaszek as a code owner August 27, 2021 12:05
@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Aug 27, 2021
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice!

@potiuk potiuk closed this Aug 27, 2021
@potiuk potiuk reopened this Aug 27, 2021
@potiuk
Copy link
Member

potiuk commented Aug 27, 2021

Reopened to rebuild

@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Aug 27, 2021
@potiuk
Copy link
Member

potiuk commented Aug 27, 2021

Some static checks failing

@deedmitrij
Copy link
Contributor Author

@potiuk can we put this PR on hold for a while? These are not all required changes. I need to fix few more tests and only then the commit will be fully ready.

@potiuk
Copy link
Member

potiuk commented Aug 27, 2021

Sure! Convert it to Draft :)

@potiuk potiuk marked this pull request as draft August 27, 2021 19:30
@potiuk
Copy link
Member

potiuk commented Aug 27, 2021

Just did :)

@deedmitrij
Copy link
Contributor Author

deedmitrij commented Aug 30, 2021

Just did :)

Thanks!

@deedmitrij deedmitrij marked this pull request as ready for review August 30, 2021 13:53
@deedmitrij
Copy link
Contributor Author

@potiuk I've finished with all required tests

- Change 'schedule_interval' from 'None' to '@once'
- Add 'setUp' and 'tearDown' methods for logs collecting
@deedmitrij
Copy link
Contributor Author

@potiuk could you, please, make a final review of this PR. The checks are already finished successfully

@deedmitrij
Copy link
Contributor Author

We would like to merge these changes asap

@potiuk
Copy link
Member

potiuk commented Aug 31, 2021

Looks good. Merging. Just a small comment: the ASAP word does not work in open-source :).

The ASAR ( As Soon As Ready) and WCHT (When Committers Have Time) [to review and merge among hundreds of request which can be more important] are better acronyms.

@potiuk potiuk merged commit 5007806 into apache:main Aug 31, 2021
@pytest.mark.usefixtures("helper")
@provide_gcp_context(GCP_GCS_KEY, scopes=SCOPES)
def test_run_example_dag(self):
self.run_dag('example_sheets_gcs', GSUITE_DAG_FOLDER)

def tearDown(self):
super().tearDown()
Copy link
Member

Choose a reason for hiding this comment

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

I may be missing something here... why we need this? I believe that if a method in a class is not overridden then parent method will be used, won't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added for explicit representation and in correspondence to other tests which have setUp\tearDown methods. Also it will help to quickly add some logic to these methods inside test.

@deedmitrij
Copy link
Contributor Author

Looks good. Merging. Just a small comment: the ASAP word does not work in open-source :).

The ASAR ( As Soon As Ready) and WCHT (When Committers Have Time) [to review and merge among hundreds of request which can be more important] are better acronyms.

Sorry, no pressure, it was like just info )) I didn't want to push somehow by this "asap". I meant of course if you have a time would be nice from your side to review and merge these changes ;)

@potiuk
Copy link
Member

potiuk commented Sep 1, 2021

Hey :D. no problem. Just the dynamics of open-source is very different to commercial projects, so wanted to make sure this is well understood. The "written" communication and brevity often lacks the context, so wanted to make sure the context is brought into the picture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants