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

Accepted template queries #269

Closed
mcguinlu opened this issue Sep 9, 2020 · 12 comments
Closed

Accepted template queries #269

mcguinlu opened this issue Sep 9, 2020 · 12 comments

Comments

@mcguinlu
Copy link
Member

mcguinlu commented Sep 9, 2020

I have just gone through the code review process in ropensci/software-review/issues/380, and wanted to flag two items in the guidance given via the "Approved" template which I found confusing. I've included the text from the template as blockquotes, and put my comments below.


pkgdown Instructions

If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,

Firstly, both myself and @maurolepore (the guest editor of my submission) were a bit confused as to how to set the redirect page up, seeing as the repo has been transferred across to ropensci and the old pkgdown website on my personal GitHub Pages (https://mcguinlu.github.io/medrxivr/index.html) no longer exists (I think?!). It might be worth adding some extra text to clarifiy this element in the guidance.

Secondly, the link given to the redirecting guidance (https://devguide.ropensci.org/#redirect) in the template is incorrect and sends you to the landing page for the dev guide. I think this is because you have gone for a multi-page bookdown approach and using #section only works on single page layout. The true link is https://devguide.ropensci.org/redirect.html


Appveyor Instructions

Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be AppVeyor Build Status.

The second thing that I found confusing was that pacakge authors are encouraged to keep their AppVeyor badge but to have it point towards the original person/package path. Does this not mean that any future pushes to the ropensci/pacakge repo will not trigger a new Appveyor build, and that this badge will always have whatever status it did upon transfer, potentially introducing confusion (particularly if other builds are failing)?


This is just my subjective experience as someone who has just gone through the code review process, but feel free to ignore any/all of it if I've missed something obvious! Happy also to make some of the smaller changes (e.g. the link) via a PR, but wanted to check in first.

Luke

@maelle
Copy link
Member

maelle commented Sep 10, 2020

Thank you, this is very useful feedback!

* set up github.com/original-owner/pkg with a gh-pages that has the redirect explained at https://devguide.ropensci.org/redirect.html
* make the README of github.com/original-owner/pkg point to github.com/ropensci/pkg
* make sure the remote URL is updated in the local clone.
  • Reg Appveyor, the URL change reflects the change of GitHub organization, what we want to avoid is changing the Appveyor organization if that makes sense? Appveyor will have picked up the new repo location and the transfer hasn't destroyed the hook (that sends your commits to Appveyor). How do you think we could improve the phrasing? [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname) if you make any push, you should see the badge links to the correct most recent build, if not please tell me.

@jeroen
Copy link
Member

jeroen commented Sep 10, 2020

Here is a trick: you can setup the redirect from your main user gh-pages repo:

@mcguinlu
Copy link
Member Author

Thanks @maelle and @jeroen for this!

First two points are great - I have tried the solution recommended by @jeroen and it worked very smoothly (see https://mcguinlu.github.io/medrxivr).

Just on the Appveyor point, I have now removed the Appveyor badge/.yml from the repo, because when I was pushing commits to ropensi/medrxivr, it wasn't triggering a build on my personal AppVeyor (https://ci.appveyor.com/project/mcguinlu/medrxivr). You can see this because the last commit built for Appveyor was cc2274f8, but in the commit history there are several other commits (see commits on Sep 8th, 2020, here) pushed before I removed the badge/.yml on Sep 9th in commit ffaa399. So maybe I did something wrong and the hook was lost during transfer?

It's not a huge issue to me, as a lot of my testing is done by GitHub Actions anyway, but it might be worth investigating in case future submission rely on AppVeyor.

And just to make it very clear, I think the dev guide/code review is an amazing resource - these points are really minor issues 😀

@maelle
Copy link
Member

maelle commented Sep 10, 2020

I'll update the guidance reg the redirects then, thanks for trying this?

Reg Appveyor why did you remove your Appveyor configuration file?
I remember being told by Appveyor support itself that one might have to re-add the repository (like what you did the very first time you activated Appveyor), that might be that?

Of course, if you now use GitHub Actions, you don't need Appveyor?

These are not minor issues if they make the transfer cumbersome, so thank you for updating this thread!

@mcguinlu
Copy link
Member Author

I got rid of it because (a) I couldn't work out how to trigger new builds through pushing to the new ropensci/medrxivr repo, and was concerned that the AppVeyor badge (which would always show the package passing) would cause confusion; and (b) as you rightly point out, having both GitHub Actions and AppVeyor might be overkill! I've kept Travis as it updates the codecov, and I haven't worked our how to do this from GitHub Actions yet!

I remember being told by Appveyor support itself that one might have to re-add the repository (like what you did the very first time you activated Appveyor), that might be that?

I'm not sure how to do that, because if I delete it to re-add it, will it be able to find the mcguinlu/medrxivr repo, seeing as this no longer exists?

As a test, I re-added the appveyor.yml build file, and could start a build on the latest commit to the ropensci/medrxivr repo manually from AppVeyor - but it is not building automatically on each push.

@Bisaloo
Copy link
Member

Bisaloo commented Sep 10, 2020

To update codecov with GitHub actions, you can use usethis::use_github_action("test-coverage").

@Bisaloo
Copy link
Member

Bisaloo commented Sep 10, 2020

To stay on topic, I remember having a lot of issues with Appveyor as well when I transferred lightr. It was not obvious to me what should be updated or not to follow the guidelines while keeping everything working. And I ended up with the same issue of Appveyor not running on new pushes. It's one of the main reasons I switched to GitHub Actions. Unfortunately, it was some time ago so I don't have a very good recollection of this...

@mcguinlu
Copy link
Member Author

To stay on topic, I remember having a lot of issues with Appveyor as well when I transferred lightr.

So it's not just me - phew!

And thanks for sharing the GitHub Action for codecov - I hadn't realised there was one!

One solution would be to use the dev guide to encourage people to use GitHub Actions from the off? In my (limited) experience, it is substantially easier/more reliable to use than having to configure the relevant .yml files.

@maelle
Copy link
Member

maelle commented Sep 10, 2020

Reg Appveyor here's an excerpt from an email exchange I had with them 2 years ago after I had assessed many webhooks no longer worked.

  

    If you believe that you need to re-create webhook, you can find correct Webhook URL on General project settings in AppVeyor UI. Use it when creating a Webhook on GitHub. Select “Push” and “Pull request” events. You can also delete unneeded webhook manually on GitHub. Or you can create new project on AppVeyor and correct webhook will be created automatically.

 

    If repo owner changed, you can may also need to re-authorize GitHub at https://ci.appveyor.com/account.

(On Appveyor one can add a new project even if it's not under one own's account, one just needs to have access to that repo IIRC).

Maybe I should change the order in https://devguide.ropensci.org/ci.html#whichci indeed, and add details about GHA.

@mcguinlu
Copy link
Member Author

My vote (if I get one?) would be to encourage devs to use GHA - it seems like what is going to become the standard in the near future, so it would future proof the guide, and it means that the testing/pkgdown/codecov can all be run from one place, rather than having three different services!

@maelle
Copy link
Member

maelle commented Sep 10, 2020

of course you get a vote.

I'll try to update the guidance with all these things tomorrow.

@mcguinlu
Copy link
Member Author

Great - I more wasn't sure if it was just an editorial thing that you'd need to discuss!

I'll close this now, as I think everything has been addressed!

maelle added a commit that referenced this issue Sep 11, 2020
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

No branches or pull requests

4 participants