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 option to specify username when deploying via ssh #1285

Merged
merged 15 commits into from
Apr 16, 2020

Conversation

chrisbrahms
Copy link
Contributor

This deals with the case of a username being specified in the the repository url in deploydocs. That is, if one calls deploydocs like this:

deploydocs(
    repo = "user@host:/path/to/repo",
)

deploying now works, rather than trying to push to git@user@host:/path/to/repo.

This came up when hosting documentation on a webserver where the git user already exists but I can't get access to it.

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.

Good idea!

@@ -474,7 +474,7 @@ The documentation are placed in the folder specified by `subfolder`.
function git_push(
root, temp, repo;
branch="gh-pages", dirname="", target="site", sha="", devurl="dev",
versions, forcepush=false, deploy_config, subfolder,
versions, forcepush=false, deploy_config, subfolder, username="git"
Copy link
Member

Choose a reason for hiding this comment

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

username is not used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was left over from an earlier attempt, thanks for catching it

host = match(r"(.*?)[:\/]", repo)[1]
# The upstream URL to which we push new content and the ssh decryption commands.
upstream = "git@$(replace(repo, "$host/" => "$host:"))"
end
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion: it feels like it should be possible to do this with just a single regex, e.g.:

match(r"(?:([^@]*)@)?(.*?)[:\/]", "user@foo.com/blah.git")

And if the first capture group is nothing, then use the default username.

Copy link
Member

Choose a reason for hiding this comment

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

Also, could we refactor this into a function (returning a user, upstream tuple?) and add some unit tests, just to make sure that the code does exactly what it is supposed to?

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, that is much more elegant (my feeble regex skills failed me). Where would be a good place to put the unit tests?

@chrisbrahms
Copy link
Contributor Author

I've factored out the repo-disassembly into user_host_upstream and added some tests. These are currently in test/deployconfig.jl--is that the correct place?

Regarding the new function user_host_upstream: to make strict happy, should I add a @docs entry for it or just remove the docstring?

src/Documenter.jl Outdated Show resolved Hide resolved
test/deployconfig.jl Show resolved Hide resolved
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. Just added a two more test cases.

src/Documenter.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Member

Regarding the new function user_host_upstream: to make strict happy, should I add a @docs entry for it or just remove the docstring?

We can add it here: https://github.com/JuliaDocs/Documenter.jl/blob/master/docs/src/lib/internals/documenter.md

@mortenpi
Copy link
Member

Good to go as far as I can tell. Thanks for the contribution @chrisbrahms!

@mortenpi mortenpi merged commit 0e4b024 into JuliaDocs:master Apr 16, 2020
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