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

Manual only trigger refactor #246

Merged
merged 4 commits into from
Oct 4, 2018
Merged

Manual only trigger refactor #246

merged 4 commits into from
Oct 4, 2018

Conversation

cicdw
Copy link
Member

@cicdw cicdw commented Oct 3, 2018

Closes #83 w/ a new PAUSE signal

Also separates out never_run and manual_only: never_run now raises DONTRUN and is a more aggressive signal than manual_only, which raises PAUSE causing inputs to be cached in the task_runner.

Additionally, adds a command line option to pytest: pytest --airflow will run the airflow tests (assuming you have airflow installed), otherwise they are skipped by default. These tests are slow and it was kind of annoying on my machine to have them run all the time.

@cicdw cicdw requested a review from jlowin as a code owner October 3, 2018 17:54
@jlowin
Copy link
Member

jlowin commented Oct 3, 2018

+1 for using a signal to indicate the flow should pause instead of parsing the exception for manual_only, but I'm not following the use case for having both manual_only and never_run as two separate triggers. Should we just get rid of one of them entirely?

Second, should --airflow flag be provided in CI to run all tests?

@cicdw
Copy link
Member Author

cicdw commented Oct 3, 2018

Re: never_run: I'm not quite sure I had a use case in mind honestly. If that adds unnecessary granularity, happy to remove it and / or go back to aliasing it to manual_only.

For the --airflow flag, we could but those tests really take a long time (2.5 minutes vs. 22secs currently) and also would require us to install airflow at some point. I think at this moment I vote we leave it as is, but maybe when we start thinking about nightly test runs (integration tests w/ server) we can include these tests. Willing to include them though, but

@jlowin
Copy link
Member

jlowin commented Oct 4, 2018

Let's kill never_run -- it's not a particularly useful name.

@cicdw cicdw mentioned this pull request Oct 4, 2018
@jlowin
Copy link
Member

jlowin commented Oct 4, 2018

PAUSE is a cool signal. Interestingly, it could be raised by any code, not just the trigger. That could be a very powerful extension.

@cicdw cicdw merged commit ca89724 into master Oct 4, 2018
@cicdw cicdw deleted the manual-only branch October 4, 2018 21:23
@jlowin jlowin mentioned this pull request Oct 7, 2018
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.

None yet

2 participants