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

ENH: Support pushing to *.github.io from other repo. #190

Merged
merged 5 commits into from
Aug 22, 2017

Conversation

danielballan
Copy link
Contributor

Doctr already supports publishing to the master branch of an org/org.github.io repo. I believe it does not support pushing from org/some_repo (and perhaps multiple other repos) into org/org.github.io. (Why do this? Different projects in an org can publish into subdirectories of one org-wide gh-pages repo and get a nice URL structure: org.github.io/repo_a, org.github.io/repo_b, etc. You could do this with separate gh-pages branches, but it's nice to keep the large build projects off in their own org-wide repo and keep the repos' branches light. The projects matplotlib, cmocean, and trackpy all do this.)

The issue arises from deploy_branch beingmaster. Local master refers to the history of org/some_repo, while doctr_remote/master refers to the history of org/org.github.io and doctr mixes them up.

I have resolved this by doing the local work on a special branch with an unambiguous name that tracks doctr_remote/{deploy_branch} and is explicitly pushed to it.

I think this modification does not break existing use cases but someone more familiar with the codebase should take a careful look. This Travis build shows it operating successfully for my use case of off this PR.

@asmeurer
Copy link
Member

Tests fail (pyflakes).

I'll push the branch up to origin so that it tests the deploy (by the way, this is the best branch name I've ever seen).

if new_deploy_branch or local_deploy_branch_exists:
run(['git', 'checkout', deploy_branch])
else:
run(['git', 'checkout', '-b', deploy_branch, '--track', 'doctr_remote/{}'.format(deploy_branch)])
Copy link
Member

Choose a reason for hiding this comment

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

Does this still automatically create gh-pages on the remote if it doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if I understand correctly that part happens in create_deploy_branch which is still called, just above the first line I touched. To be clear I have not tested that; I just don't think the behavior had changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I was wrong about that -- it did not successfully create a master branch if one did not exist -- but that is fixed in 8007280, which I think will pass once it runs in context where it's allowed to push.

@asmeurer
Copy link
Member

Seems like this would have needed to have been done in the first place for .github.io support to be working.

@asmeurer
Copy link
Member

I'd like to see a deploy added to .travis.yml to deploy to drdoctr.github.io (it seems that wasn't done in #168).

@danielballan
Copy link
Contributor Author

Seems like this would have needed to have been done in the first place for .github.io support to be working.

Seemed that way to me too but I figured someone would have caught it by now? I'll try to add that test you suggest.

@danielballan
Copy link
Contributor Author

I amended my original commit to fix the pyflakes failure and added a deployment to drdoctr.github.io.

@danielballan danielballan force-pushed the other-master branch 8 times, most recently from a3cfb58 to 8007280 Compare May 23, 2017 14:49
@danielballan
Copy link
Contributor Author

I think this will pass if it runs on the upstream fork.

@danielballan
Copy link
Contributor Author

danielballan commented May 23, 2017

(The failure is: we can't create a branch that tracks the remote doctr_remote/master if it doesn't exist, and we can't create that remote if canpush is False.)

@danielballan
Copy link
Contributor Author

Please let me know when you have the bandwidth to review/merge and I will happily rebase.

@asmeurer
Copy link
Member

I can review, but I think this PR was not working (at least it was failing the tests).

If we are deploying *to* a github.io repo we must deploy to 'master' (GH does
not even let you configure this -- it's always 'master') regardless of whether
we are publishing content *from* a *.github.io repo or some other repo. Thus,
deploy_repo, not deploy_dir.
Use the working branch approach in branch creation as well.

Former output (from Travis failure):

```
Creating master branch
git checkout --orphan master
fatal: A branch named 'master' already exists.
```
@danielballan
Copy link
Contributor Author

@asmeurer The Travis failure will go away when this is merged into master, where canpush is True, and doctr_remote/master can be created. In this PR branch, canpush is False, so we we can't create doctr_remote/master and therefore we cannot create a local branch that tracks it. (This error happens here in the build log.)

There is a working example you can see: I have been deploying NSLS-II docs using the version of doctr in this branch. See a working build here, which published this commit. This particular build ran after my rebase, verifying that the rebase didn't break it.

@asmeurer
Copy link
Member

Well we've got it set up to not make PRs mergeable unless they pass, so I've gone ahead and created the master branch.

@asmeurer
Copy link
Member

Now it failed:

git fetch doctr_remote
Checking out doctr working branch tracking doctr_remote/master
git checkout -b __doctr_working_branch --track doctr_remote/master
fatal: Cannot update paths and switch to branch '__doctr_working_branch' at the same time.
Did you intend to checkout 'doctr_remote/master' which can not be resolved as commit?

By the way, we should put an except block in our try/finally that prints an exception message to make it easier to find the actual error in the build logs.

@danielballan
Copy link
Contributor Author

Thanks. The ball is in my court then. I'll see if I can get the tests to pass.

@danielballan
Copy link
Contributor Author

@asmeurer It looks like the failing builds (the ones in travis-ci/pr) haven't run since yesterday, before the master branch was created. Would you re-run them? I don't think I have the necessary permissions to do that.

@danielballan
Copy link
Contributor Author

Ah, pushing a new (empty) commit made Travis spin. Tests pass. Now let me remove that empty commit....

@asmeurer
Copy link
Member

I did a merge on master after merging another PR, which should make it re-run. I'll merge this is Travis passes.

@danielballan
Copy link
Contributor Author

What sort of error message did you have in mind for that try/finally block? Do you want something with caps or whitespace to stand out in the logs? Or is there something slicker we can do?

@asmeurer
Copy link
Member

I'm working on the message at #239. If you have a better suggestion let me know.

@asmeurer asmeurer merged commit c9f25d4 into drdoctr:master Aug 22, 2017
@asmeurer
Copy link
Member

OK, let's see if https://github.com/drdoctr/drdoctr.github.io gets updated.

@asmeurer
Copy link
Member

Oh I need to run doctr configure and actually set up the deploy key. But why didn't the build fail? https://travis-ci.org/drdoctr/doctr/jobs/267309777

@asmeurer
Copy link
Member

I think maybe it's because we already have doctr_remote from the other doctr commands. It too should be cleaned up in the finally block.

@asmeurer
Copy link
Member

Oh nevermind that's already taken care of. The docs aren't pushed because it thinks they haven't changed.

@danielballan danielballan deleted the other-master branch August 22, 2017 19:23
@asmeurer
Copy link
Member

OK, I think you need to clear the __doctr_working_branch in the finally block. I'll open a PR.

@asmeurer
Copy link
Member

No, you're already doing that too. I'm not seeing why it's failing.

@asmeurer
Copy link
Member

Oh, I see. It's because we (probably stupidly) have it set up in our Travis that you have to have --sync for the docs to actually push.

@asmeurer
Copy link
Member

Which actually leads to a second question, which is how we can actually test this, since doctr is currently set up to use only one deploy key, but we would need two.

I guess I can generate a new deploy key manually and use it for both repos (hopefully GitHub allows that).

@asmeurer
Copy link
Member

No, GitHub doesn't allow it.

@danielballan
Copy link
Contributor Author

Right. Is it worth adding something to the test that makes a tiny random change to the docs each time?

@asmeurer asmeurer mentioned this pull request Aug 22, 2017
@asmeurer
Copy link
Member

The docs do get updated each time (sphinx adds timestamps). So that isn't necessary. I added --sync here, but it won't work until we can get a deploy key set up, which we need to figure out how to do (#242).

@danielballan
Copy link
Contributor Author

Ah, got it.

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