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.

Sign up for GitHub

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

Jump to bottom

Drop SSH keys from env when running external commands #1962

Merged
merged 1 commit into from
Oct 16, 2022
Merged

Drop SSH keys from env when running external commands #1962

merged 1 commit into from
Oct 16, 2022

Conversation

fredrikekre
Copy link
Member

Should fix #1958.

@fredrikekre fredrikekre requested a review from mortenpi October 6, 2022 13:05
@fredrikekre fredrikekre force-pushed the fe/run-env branch 2 times, most recently from 93fcd6d to 0a65a57 Compare October 6, 2022 13:25
mortenpi
mortenpi reviewed Oct 7, 2022
View reviewed changes
Copy link
Member

@mortenpi mortenpi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It wouldn't save us from these cases though (there are probably more)?

Documenter.jl/src/Utilities/Utilities.jl

Line 777 in 70017e0

read(pipeline(cmd; stderr = stderr_output), String)

In general, I think doing something like this is worth it, but I think in addition we should also fix the places where we print the Cmd objects in errors.

Documenter.jl/src/Utilities/Utilities.jl

Line 431 in 70017e0

readchomp(`$(git()) rev-parse HEAD`)

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

Perhaps we should also recommend splitting it up in two steps and only provide the secret in the yml file to the deploydocs part.

@mortenpi
Copy link
Member

mortenpi commented Oct 10, 2022 •
edited
Loading

Perhaps we should also recommend splitting it up in two steps and only provide the secret in the yml file to the deploydocs part.

This was also mentioned here: #1959 (comment)

I don't think it's a bad idea. The only question I have though is whether we should then move towards having more of the decision whether to deploy or not in the CI config, as opposed to decided by deploydocs?

@fredrikekre fredrikekre requested a review from mortenpi October 10, 2022 20:52
@fredrikekre
Remove DOCUMENTER_KEY when not needed
3cadd7b 
This patch removes DOCUMENTER_KEY from the environment when it is not
needed. In particular, it is removed from all of makedocs(...) and
removed from all git commands in deploydocs, fixes #1958.
mortenpi
mortenpi approved these changes Oct 16, 2022
View reviewed changes
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

@fredrikekre fredrikekre merged commit 998d26e into master Oct 16, 2022
@fredrikekre fredrikekre deleted the fe/run-env branch October 16, 2022 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortenpi mortenpi mortenpi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reporting process errors leaks DOCUMENTER_KEY
2 participants
@fredrikekre @mortenpi