ctv icon indicating copy to clipboard operation
ctv copied to clipboard

Improve handling of events that aren't pull requests

Open willgearty opened this issue 1 year ago • 8 comments

Currently, it's recommended that the workflow be set to run if a push OR pull_request is made. However, if a push was made to the main branch and there were any new packages that were archived on CRAN, these wouldn't be caught because the script checks against the version on the main branch (so there would never be a diff of archived packages). Now, if the event that triggered the workflow is not a pull request, the workflow fails if there are ANY archived packages, regardless of what the version on main looks like. This also now makes it work for scheduled jobs, so I've added the recommendation that the workflow be scheduled to run once a month.

willgearty avatar Dec 09 '24 22:12 willgearty

I tested this here: https://github.com/cran-task-views/Phylogenetics/actions/runs/12244353596/job/34155911308#step:9:39

willgearty avatar Dec 09 '24 22:12 willgearty

@tuxette @zeileis thoughts on merging this?

willgearty avatar Feb 14 '25 14:02 willgearty

Hi @willgearty ! Sorry: I missed your pull request... I guess that depends on how people organize their repository but:

  • I indeed expect the pull request to fail if there is any package archived on it, whatever the version of main (to warm me that I missed a package has been archived). This is, of course, not perfect because you might want to keep this package nevertheless. If I understand what you posted, pull-request will now fail only if one of the packages added in the pull-request is archived. Am I right?
  • About running automatically the check every month, I would personally use it but I think that we should at least comment on how to turn it off maybe. What do you think? @zeileis : Any thought?

tuxette avatar Feb 16 '25 16:02 tuxette

  • I indeed expect the pull request to fail if there is any package archived on it, whatever the version of main (to warm me that I missed a package has been archived). This is, of course, not perfect because you might want to keep this package nevertheless. If I understand what you posted, pull-request will now fail only if one of the packages added in the pull-request is archived. Am I right?

I think I'm fine with it working either way, I just kept the existing behavior of the pull request check. Yes, the way it's currently set up (and was set up before), a pull request with no additions of archived packages will pass, even if other packages in the task view are archived. However, now, if the repo does have existing archived packages, the check will then fail upon merging the pull request (or upon changes to the main branch or on schedule).

I could see this overall behavior being a little confusing to users when merging a PR ("I thought this was just passing? Why is it now failing?"). However, it may also make sense because the pull request itself didn't introduce any issues, so it shouldn't cause a failed check before being merged?

willgearty avatar Feb 17 '25 15:02 willgearty

Thanks for the clarification @willgearty ! This is OK for me to merge this pull request. We will just wait a bit more to have @zeileis confirm that he agrees too.

tuxette avatar Feb 20 '25 06:02 tuxette

Re: running the check once a month. I don't think that this is necessary. If there are any errors on the main branch I will be notified from CRAN through the daily updates. If there are no errors but archived packages, then Kurt will open an issue after 60+ days. So my feeling is that it is not urgent to act upon CRAN archivals earlier than that.

Re: when/how to fail during PRs. I find this hard to judge because personally I would look at the details of the check anyways (and not just trust an "ok" vs "failed" flag). Hence I agree that the check run on PRs should only fail if the PR adds non-existent or archived packages. For the remaining details I'm not so sure.

zeileis avatar Feb 20 '25 07:02 zeileis

Which "daily updates" are you referring to? I don't get daily updates from CRAN for my task views... And are Kurt's issues automated in some way? Perhaps this could replace whatever system he has in place...

willgearty avatar Feb 20 '25 17:02 willgearty

The daily updates on CRAN are run automatically in a cronjob. The summary report of that cronjob just goes to me and in more than 95% of cases I don't need to do anything.

Kurt's issues are semi-automated. He triggers them about once a week. The plan is to fully automate the process when we have the feeling everything goes smoothly enough.

zeileis avatar Feb 20 '25 19:02 zeileis

Will, we never properly resolved this so I'm closing this here.

Note that we have improved the validate-ctv action today. It now uses rocker/r2u to speed up the performance. More importantly, regarding the issue raised in this PR, the check for archived packages was completely rewritten. Rather than comparing against some version of the .md file on GitHub, it now always compares against the archived packages listed on CRAN. See: https://github.com/cran-task-views/ctv/blob/main/validate-ctv/action.yml#L53-L64

If this doesn't resolve the problem, we should discuss an improvement of the new action in a separate issue or PR.

zeileis avatar Sep 24 '25 00:09 zeileis

Sounds like that should address my concern. I'll try out the new version and let you know if I find any issues or have any lingering concerns.

willgearty avatar Sep 24 '25 01:09 willgearty

Great, thanks! :+1:

zeileis avatar Sep 24 '25 01:09 zeileis