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

Update saved RDS for the removal of a workflow #19

Merged
merged 2 commits into from
May 6, 2024

Conversation

IndrajeetPatil
Copy link
Member

R-CMD-check-strict was recently removed

@IndrajeetPatil
Copy link
Member Author

@etiennebacher Any ideas on what's going on with credentials issue here? I think we are using @rempsyc's credentials?

@etiennebacher
Copy link
Member

Oh no, not this issue again 😅 Apparently the workflow runs fine every 8h on main, maybe it's just a temporary issue? I think you can merge this

@IndrajeetPatil IndrajeetPatil merged commit d81b0ef into main May 6, 2024
1 check failed
@IndrajeetPatil IndrajeetPatil deleted the rm-strict-workflow branch May 6, 2024 12:46
@IndrajeetPatil
Copy link
Member Author

The dashboard hasn't been rendered since this PR was merged 😢

@etiennebacher
Copy link
Member

Actually I can reproduce locally, it fails on this URL: https://github.com/easystats/insight/workflows/R-CMD-check-main/badge.svg
which doesn't exist. I didn't follow changes on insight repo, @strengejacke was this workflow removed?

@etiennebacher
Copy link
Member

I'm confused, R-CMD-check-main is in the output of extract_gha_workflows("easystats", "insight") but not extract_gha_workflows("easystats", "datawizard") for example

@IndrajeetPatil
Copy link
Member Author

IndrajeetPatil commented May 8, 2024

Yeah, Daniel has changed workflows in his repos: easystats/insight@ba37896

We need to figure out how to deal with this in the dashboard now. This is why I didn't want every developer to do make arbitrary changes to the infrastructure; it's impossible to build tooling that works across all repos of interest now. We need to keep carving out exceptions.

Another option is to just get rid of this tab showing workflow statuses. It hasn't been that reliable anyway.

@strengejacke
Copy link
Member

I'd suggest we reduce workflows on main to a minimum (pkgdown, win/Ubuntu devel?) and only run the larger test suite only for PRs.

Furthermore, we should remove the workflows for old release, as these don't work anyway.

WDYT?

@etiennebacher
Copy link
Member

I'd suggest we reduce workflows on main to a minimum (pkgdown, win/Ubuntu devel?) and only run the larger test suite only for PRs.

This sounds reasonable to me but ultimately I think @IndrajeetPatil is most familiar with the workflows repo so my opinion is not that important.

Furthermore, we should remove the workflows for old release, as these don't work anyway.

I don't know about those failures, I understood there were some issues with Matrix and other important packages in the CI (as always). What's weird is that they fail for oldrel but not oldrel-2 (in datawizard at least).

@IndrajeetPatil
Copy link
Member Author

@strengejacke I disagree with the part that we don't need to run the workflows on the default branch.

The true status of our codebase is the default, and not the status, branch, and we must make sure that the workflows are passing there, if not for every commit than at least on a daily or weekly basis. Otherwise, things can break and you will never know about it in an inactive repo because no workflows are being run, which is exactly the case for the reps you maintain. If we were using dependency management system like renv, it wouldn't be that big of an issue. But we don't.

The fact that it fails on R 4.0 but not later versions is a technical problem that can be easily solved. Ripping out a big chunk of the entire CI just because of that is throwing the baby out with the bathwater.

@strengejacke
Copy link
Member

which is exactly the case for the reps you maintain

I don't understand your argumentation. For every change to the repo, we open a PR. All tests are run on that PR. We then merge the PR, and now all the same tests are immediately run again on main. Why? Those checks on main have no additional benefits, as they would replicate the checks results we had seconds before on PR before merging.

@IndrajeetPatil
Copy link
Member Author

You are assuming a lot here. PRs are rarely reviewed immediately after they are made, let alone merged.

Here are some hypothetical scenarios to give you a more tactile feel for how CI passing on PR is no guarantee that it will also pass on the default branch (unless you are using a reproducible environment like renv, which we don't):

  • You make a PR on April 23, all builds pass, and it's approved on the 24th. The PR is merged on the 25th, and the builds fail because R 4.4 came out on April 24. There are binary incompatible changes in some dependencies, the graphics engine has changed and so the graphical snapshots are not comparable, etc.
  • [Insert events of interest] and the builds fail because there was an update in some package between creation and merge of PR
  • [Insert events of interest] and the URL check is failing because some resource was removed
  • etc.

In such cases, since we are not running all workflows on the default branch, these problems are going to be discovered only when a PR is made. Also, this is going to be a rude surprise for the next person who makes a PR because they now need to fix things whose breakage has nothing to do with their PR.

A lot of these problems can be avoided by using renv. But I would much rather always work with the most recent version of our dependencies to detect and fix any breakage due to a package update because doing so is quick and cheap on GitHub, but not on CRAN. Additionally, we have no control over what our users are doing: even if we use renv in our development workflow, nothing prevents our users to upgrade to recent version of deps and detect breakages or regressions. This is why I have configured all workflows to always upgrade to use the latest version of deps.

@strengejacke
Copy link
Member

I see the point that the time gap between pushing to a PR, reviewing and merging can cause new problems that haven't been detected due to checks that are already finished. However, once we merge and everything is OK on main, there can also be changes in between that we do not detect because our checks are only started again if we create a new PR - so that's no big difference, anyway. And, that's why I suggested running not all, but some important checks on main after merging, so it's likely we find any serious issues. We don't need to run all checks again. Or, the alternative would be a cronjob that runs all checks once a week or so on main.

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.

3 participants