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 lint CI race condition #1081

Merged
merged 8 commits into from
May 17, 2019
Merged

fix lint CI race condition #1081

merged 8 commits into from
May 17, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented May 13, 2019

Lint CI requires install_test and was sometimes failing due to a race condition.

fixed!

@djrtwo djrtwo requested review from protolambda and hwwhww May 13, 2019 21:03
@hwwhww
Copy link
Contributor

hwwhww commented May 14, 2019

lint job doesn't require install_test (make install_test). Instead, it installs flake8 with make intall_lint.

But it seems we should make it requires checkout_specs.

      - lint:
          requires:
            - checkout_specs

@djrtwo
Copy link
Contributor Author

djrtwo commented May 14, 2019

neither install_test nor checkout_specs actually build the pyspec

Here is an instance of the error. Because test builds the pyspec, I think we should just have it as a dependency of test. Thoughts?

@hwwhww
Copy link
Contributor

hwwhww commented May 16, 2019

@djrtwo Ahh, you're right about that both test and lint jobs build pyspec. I also realized that only install_test does save_cached_venv so flake8 installation isn't part of the stored cache! I updated it to install_env job.

If we make lint requires test, the workflow becomes:

image

Sorry for the spam commits of the experiment on this branch. I was thinking about if we can utilize parallel jobs to:

image

But it seems to require reworking Makefile... so I'm inclined to keep serial workflow for now.

@djrtwo djrtwo merged commit 421687c into dev May 17, 2019
@djrtwo djrtwo deleted the lint-race branch May 17, 2019 15:07
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.

2 participants