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 support for pushing PR previews to a different branch and/or different repo #1307

Merged
merged 4 commits into from May 4, 2020
Merged

Conversation

DilumAluthge
Copy link
Contributor

@DilumAluthge DilumAluthge commented May 2, 2020

Currently, the only way to have PR previews is to put them in the previews/PRXX subdirectory in the same branch of the same repository as your actual docs. For example, if your actual docs are pushed to the gh-pages branch of the MyPackage.jl repo, then the PR preview for PR number 123 is pushed to the previews/PR123 subdirectory in the gh-pages branch of the MyPackage.jl repo.

But, for a variety of reasons, you may not want to keep your PR previews in the same branch and/or repo as your actual docs.

This PR adds the ability to customize the branch and/or repo to which PR previews are pushed. The options are:

  • Same repo, same branch - Prior to this PR, this is the only option. In this PR, we keep it as the default behavior.
  • Same repo, different branch
  • Different repo, same branch name
  • Different repo, different branch name

We also add support for the DOCUMENTER_KEY_PREVIEWS environment variable. If the DOCUMENTER_KEY_PREVIEWS environment variable is set, then that will be the SSH key that is used for pushing PR previews. If the DOCUMENTER_KEY_PREVIEWS environment variable, then the regular DOCUMENTER_KEY environment variable will be used.


Example


Prior art

This is quite easy with Franklin. See e.g. my setup here:

@DilumAluthge
Copy link
Contributor Author

This is ready for review @mortenpi @fredrikekre

@DilumAluthge
Copy link
Contributor Author

Bump @mortenpi

@mortenpi mortenpi added this to the 0.25.0 milestone May 2, 2020
@mortenpi
Copy link
Member

mortenpi commented May 2, 2020

I haven't gone through the code in detail yet but glancing at it, it looks good! Will try to go through this within the next few days.

Could you also add a note into CHANGELOG.md?

@DilumAluthge
Copy link
Contributor Author

Could you also add a note into CHANGELOG.md?

Done!

Let me know if the changelog entry is too verbose.

@DilumAluthge DilumAluthge reopened this May 3, 2020
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM as is!

One tiny request: could we have a test in test/deployconfig.jl that makes sure that DOCUMENTER_KEY_PREVIEWS gets properly fetched from the environment too (and gets ignored when it is in the env. but it's not a preview build)? That's the only thing that I saw that wasn't being tested at the moment.

src/deployconfig.jl Outdated Show resolved Hide resolved
DilumAluthge and others added 2 commits May 3, 2020 20:52
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@DilumAluthge
Copy link
Contributor Author

One tiny request: could we have a test in test/deployconfig.jl that makes sure that DOCUMENTER_KEY_PREVIEWS gets properly fetched from the environment too

This part is done.

(and gets ignored when it is in the env. but it's not a preview build)

This will be hard. The logic for this part is in the git_push function, so to test it we'd need to have some kind of integration test harness.

@mortenpi
Copy link
Member

mortenpi commented May 4, 2020

(and gets ignored when it is in the env. but it's not a preview build)

This will be hard. The logic for this part is in the git_push function, so to test it we'd need to have some kind of integration test harness.

Ah, that's right, I didn't look at it closely enough. In that case, I'd say it's good to go. Thanks for the contribution!

For planning purposes: would it be fine for you to use Documenter master for a while? I would definitely like to have this in a minor release, but there are a few other things I would like to get into 0.25, so it might be a few weeks before a release.

@mortenpi mortenpi merged commit 51c2b29 into JuliaDocs:master May 4, 2020
@DilumAluthge DilumAluthge deleted the dpa/previews-repo-branch branch May 4, 2020 03:03
@@ -461,13 +484,14 @@ function deploydocs(;
@debug "running extra build steps."
make()
end
@debug "pushing new documentation to remote: '$repo:$branch'."
@debug "pushing new documentation to remote: '$deploy_branch:$branch'."
Copy link
Member

Choose a reason for hiding this comment

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

deploy_repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call

Copy link
Member

Choose a reason for hiding this comment

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

Also :$deploy_branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! #1310

@DilumAluthge
Copy link
Contributor Author

DilumAluthge commented May 4, 2020

For planning purposes: would it be fine for you to use Documenter master for a while? I would definitely like to have this in a minor release, but there are a few other things I would like to get into 0.25, so it might be a few weeks before a release.

Sounds good! If you remember, just ping me whenever you make the release, so that I can remember to switch from using Documenter master to using Documenter 0.25.

@fredrikekre
Copy link
Member

The docs for how this interface should be used also needs to be updated.

@mortenpi
Copy link
Member

mortenpi commented May 4, 2020

The fallbacks for deploy_folder here should also return DeployDecision -- currently deploydocs throws an error.

@fredrikekre, I guess the related docstring is the one you mean that should get updated?

@fredrikekre
Copy link
Member

Yes.

@DilumAluthge
Copy link
Contributor Author

#1315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants