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

Ssh vignette #67

Closed
wants to merge 6 commits into from
Closed

Ssh vignette #67

wants to merge 6 commits into from

Conversation

hansvancalster
Copy link
Contributor

You might want to merge this into another branch...

@hansvancalster
Copy link
Contributor Author

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

@florisvdh
Copy link
Member

Interesting @hansvancalster. I didn't look in detail but it seems that most of this is not specific to checklist, am I right? E.g. the same thing is needed to compile an Rmarkdown file that calls git2r directly.

In that case it may be better to add it as a general tutorial on the tutorials website. I suspect it would even solve inbo/tutorials#26, if I understand that issue right.

That's just a suggestion, and even if it would indeed move to tutorials it would still be a smart thing to mention the need here.

@hansvancalster
Copy link
Contributor Author

@florisvdh I wouldn't place it in tutorials (although it is indeed generic). The reason is that tutorials should reflect best practice and best in class packages. I believe we should move from git2r to gert for that reason. The developers of that package have gone to great lengths to resolve problems of authentication with Git / GitHub on different OS.

In the longer run, it would probably also be best for the checklist package to switch from git2r to gert - but as @ThierryO pointed out, this is indeed a major change.

A tutorial on passwords in GIT is still needed, but it would rather promote the HTTPS protocol instead of the SSH protocol (as tidyverse developers (especially packages gert, gh, usethis) and GitHub do.
I wrote the vignette in this PR as a service to users who need to switch to the SSH protocol for some checklist functions that currently use git2r under the hood.

For an in-depth discussion of this, you can read the blog post when usethis released version 2.0.0 (which was mainly the switch from git2r to gert).

@ElsLommelen
Copy link
Contributor

but it would rather promote the HTTPS protocol instead of the SSH protocol (as tidyverse developers (especially packages gert, gh, usethis) and GitHub do.

I thought @ThierryO rather wanted to promote SSH instead of HTTPS at INBO? See here in the last half of the post.

Anyway, I also planned on writing a similar vignette for package forrescalc (which also uses git2r), so I am in favour of @florisvdh 's idea to make it a general tutorial which could be linked to from the packages where needed. (And I am very happy to see the job is done for me, apparently postponing can have advantages. ;-) ) Independent of the discussion on which protocol we should promote at INBO, I think it could be useful to have a reference to both protocols in our tutorials and a short explanation of the difference (e.g. advantages and disadvantages). Probably at least some users will for some reason need the other protocol than the one that is promoted, and it is nice to find an easy and trusted reference to a manual in this case.

By the way, thinking of tutorials and r_universe: wouldn't it be good to add the tutorials as articles at INBO r_universe? Apparently r_universe is very good to bundle articles, and it seems nice if we can leaf/search through all vignettes and tutorials there. And the SSH manual would certainly fit there: close to the packages, easy to refer to from the packages, and accessible as a manual for any user of INBO r_universe. (While checklist is really for the advanced R user that makes his/her own packages, also starting R users may need the SSH manual, but the latter are very unlikely to know the checklist package at all...)

@ElsLommelen
Copy link
Contributor

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

@hansvancalster This seems a generic problem when installing checklist during CI. I solved it for one of my packages by adding remotes::install_cran("codemetar") before checklist is installed. I hope this helps? (It would probably be better to also fix this in the templates of checklist to solve it immediately for all packages that use checklist.)

@florisvdh
Copy link
Member

@ElsLommelen @hansvancalster thanks for sharing these useful thoughts (and for the link to the usethis blog). Let's await how Thierry sees this.

In case the vignette stays here, do note that all vignettes will still end up in the tutorials website, with a clear prefix à la 'package: title' (something @LienReyserhove will probably work on this year). Of course it's not the same as a standalone tutorial since the contents would not specifically tie to checklist in the latter case.

By the way, thinking of tutorials and r_universe: wouldn't it be good to add the tutorials as articles at INBO r_universe?

@ElsLommelen I can't judge because of limited background - you have looked better at it - anyhow this sounds good! I suggest you add your suggestion as an issue in the tutorials repo to gather further feedback and continue the discussion there.

@ElsLommelen
Copy link
Contributor

  • you have looked better at it -

Eh, @florisvdh , it looks nice in the keynote of UseR, but I must admit I haven't tested it yet in practice...

@hansvancalster
Copy link
Contributor Author

@florisvdh @ElsLommelen @ThierryO I have a PR #68 ready to switch from git2r to gert. If that PR gets merged, than this PR #67 can be closed and the vignette can become part of a more general INBO tutorial explaining both the HTTPS and SSH protocols.

@ThierryO
Copy link
Collaborator

check_package check fails in GHA (complains about missing documentation), but locally everything seems OK (locally devtools::document() does not change anything)

You probably had a different version of Roxgen2 installed on your machine. That version number is stored in the DESCRIPTION. check_document() checks is any file is changed after updating the document.

ThierryO added a commit that referenced this pull request Sep 21, 2021
…devtools::document() changed files

relevant for #67
@ThierryO
Copy link
Collaborator

If you know where to look you find it in the log file. I've tried to improve the error message in ef53c5a

@hansvancalster
Copy link
Contributor Author

If you know where to look you find it in the log file. I've tried to improve the error message in ef53c5a

Good idea to improve the error message in that way!

@hansvancalster
Copy link
Contributor Author

Now #68 is merged, this PR can be closed.

@ThierryO ThierryO deleted the ssh-vignette branch May 5, 2023 13:30
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.

4 participants