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

Don't attempt to push on builds from forks #332

Merged
merged 32 commits into from Dec 3, 2018
Merged

Don't attempt to push on builds from forks #332

merged 32 commits into from Dec 3, 2018

Conversation

asmeurer
Copy link
Member

Travis does not make encrypted environment variables available to forks, so
doctr should not attempt to push from them.

I have to check this with the GitHub API, so hopefully that doesn't cause issues.

Fixes #110
Closes #331
Closes #236

Travis does not make encrypted environment variables available to forks, so
doctr should not attempt to push from them.

Fixes #110
Closes #331
Closes #236
@asmeurer
Copy link
Member Author

The tests are on my fork. https://travis-ci.org/asmeurer/doctr/builds/443414925. It looks like there are some other issues because the doctr tests weren't designed to run on a fork build.

I think this might happen when the request fails. Since we can't really
control this, handle the common case of the repo not being a fork.
That way we can see if it is being skipped properly.
If it doesn't yet exist doctr will assume it is a file because it is syncing a
single file.
If the dst is an existing directory, or ends in /, it is treated as a
directory and the file is synced to that directory. Otherwise, it is treated
as a file and the file is synced to that filename.
@asmeurer
Copy link
Member Author

Looks like the API checks fail pretty regularly because of API limits. I'm not sure how feasible this fix is.

@asmeurer
Copy link
Member Author

We do call doctr a lot in our tests (13 tests * 2 Python versions per build), and GitHub's unauthenticated rate limite is 60 requests per hour. I don't know how that ends up being split up on Travis, but assuming that each fork gets its own IP most likely most forks wouldn't hit this issue very often.

@asmeurer
Copy link
Member Author

We can print a warning, but I'm not too happy with that, as it could confuse people. Is there really not a way to check this without using the API?

@asmeurer
Copy link
Member Author

This also contains a minor fix for syncing a single file when the deploy directory doesn't already exist (it came up because my fork has an old copy of gh-pages).

@asmeurer
Copy link
Member Author

asmeurer commented Dec 3, 2018

Going to try this. If the API call fails, it assumes that it isn't on a fork, so it may still fail on forks sometimes (or print a warning message on non-forks).

I've also got some fixes to single-file syncs which came up on my fork https://travis-ci.org/asmeurer/doctr/branches. If we do end up reverting this we should try to keep those fixes intact.

@asmeurer asmeurer merged commit b212186 into master Dec 3, 2018
@asmeurer asmeurer deleted the forks branch December 3, 2018 19:18
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

1 participant