-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Training #24
Conversation
Code Coverage Summary
Diff against main
Results for commit: 29e268b Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems like a very cool and useful tool, and probably was pretty fun to put together.
I do think there are some things that we may want to rework though, please see my comments
inst/training_202307/202307.qmd
Outdated
|
||
## Explanation for direct dependencies | ||
|
||
The initial prototype of `min` strategy included recursive find of minimal dependency. This oftentimes lead into usage of already archived package or not able to compile very old package on modern systems architecture. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a function of the system you're running your tests on, though, and not an inherent "code failure" in the sense that min is searching for.
That said we should be using versions of recursive dependencies that were concurrent with the versions of the direct deps being tested. switchr has code to do this. You'll need to transform a switcher manifest to a pak manifest, but that should be easily doable.
Testing old versions of direct dependencies with modern versions of their recursive dependencies is not a useful or realistic check, in my opinion. Easy to get breakages just from that mismatch that dont' reflect any real cohort that anyone is actually going to have in their renv/locked site library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a function of the system you're running your tests on, though, and not an inherent "code failure" in the sense that min is searching for.
True. I tried to include that in the discussions down below about min deps putthing this as it's installable set of dependencies - i.e. not necessarily code-wise minimal set of dependencies. I will try to rephrase that section again.
That said we should be using versions of recursive dependencies that were concurrent with the versions of the direct deps being tested. switchr has code to do this.
That's a very interesting idea - thanks for putting this. I haven't thought about that this way. I will have to explore it. Maybe we can make a new strategy out of it? Just thinking... I am going to put this in the future work not to miss that.
The problem that I am seeing right now is that we would have to resolve (recursively) the whole dependency tree on our end. It's of course doable but from the very beginning I wanted to keep it as a somewhat thin wrapper on pkgdepends
and relay on it when it comes to deps resolve, download, install and all of that. Putting custom steps into its workflow it's not an easy task and require so ugly workarounds (something that I did once here but I am not very proud of it). Having this fully custom also comes with a price of maintaining same interfaces and not incorporate all the edge cases that folks have already included. I will have to think about implementation but I agree that feels more correct in the sense of probability
Testing old versions of direct dependencies with modern versions of their recursive dependencies is not a useful or realistic check, in my opinion
Yes and no.
- Yes because it's very unlikely to have direct dependency on pkg of >10 years old alongside indirect dependency of few months old.
- No because it's technically possible hence some safeguards are required.
- No because it's quite likely if we are thinking about relatively new packages with many releases, e.g. in our field: citril imports chevron of version (newest - 3 releases) and we want to use the newest rtables. I can definitely imagine this scenario.
I am going to create a new slide to discuss it.
|
||
# What's needed? | ||
|
||
For **max** strategy the algorithm will look for dependent packages references in a new section `Config/Needs/verdepcheck` of the `DESCRIPTION` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my opinion, this should be an argument, but thats probably because I designed switchr that way.
If the manifest is something you pass the testing machinery, you can easily tell it to test new branches, etc, without editing the DESCRIPTION file which I hate the very concept of doing when the package has not changed. It changes the MD5, which means the package is different, even when its not different, and then you have package versions which have the same version number and different MD5s running around and its a huge awful mess that didn't need to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think it is going to be changed that frequently. A typical use that I am for-seeing would be that if once specified it would remain unchanged unless a new dependency is added / removed which will anyway involve changes of the DESCRIPTION
file.
Having said that, Config/Needs/*
is generally recommended place to store such metadata for CI that is also used by others.
inst/training_202307/202307.qmd
Outdated
|
||
- One might argue that `foo.package` is fully compatible with the `tibble@1.0.0` syntax but the truth is that `tibble@1.0.0` cannot be installed alongside `dplyr@1.0.0` hence `foo.package` cannot be used with both `dplyr@1.0.0` and `tibble@1.0.0` loaded. Therefore `tibble (>= 1.0.0)` really indicates `tibble (>= 2.1.3)` in this case. | ||
|
||
- Because of above, the minimal version specification of dependencies is the _true_ **installable** and **compatible** set of dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree here. Conceptually, foo.package
only needs tibble > 1.0.0 and dplyr 1.0.0. It does not care that dplyr 1.0.0 requires tibble 2.1.3. Thats dplyr's problem, and that requirement is already encapsulated and enforced in dplyr 1.0.0
Look at it this way. The point of this is to figure out if our packge is usable within a particular renv'ed environment, right. that renv has tibble 1.0.0, it isn't going to have dplyr 1.0.0, so we already know the answer is no. We don't get any extra information from also requiring tibble 2.1.3
Don't get me wrong, I'm a big fan of installable cohorts. I have literally written a paper and given numerous research talks about it. It was central in my promotion talk to scientist at genentech. BUT, that isn't the purpose of htis tool. No one is going to come along and say "man I really want to install the exact minimum requirements to use rlistings 0.5.3, but no later because I hate all package versions that have come out in the last 5 years!!!
This tool is for finding requirements, and in that case being discussed here, the only meaningful versioned requirement is dplyr 1.0.0. So long as you ahve that, you're guaranteed to have a version of tibble that works too, so you're good and nothing more need be stated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting this comment :) It is probably the weakest side and encapsulation and who is responsible of what resonates with me very well.
It all stems from the limitations of pkgdepends
and its resolve dependency algorithm - or to be more fair: our specific requirements.
Please have a look at the example up above - let me re-paste it again here:
x <- pkgdepends::new_pkg_deps(c("dplyr@1.0.0", "tibble@1.0.0"))
x$solve()
x$get_solution()
<pkg_solution>
+ result: FAILED
+ refs:
- dplyr@1.0.0
- tibble@1.0.0
+ constraints (93):
(...)
x failures:
* dplyr@1.0.0: Can't install dependency tibble (>= 2.1.3)
* tibble@1.0.0: Conflicts with tibble@1.0.0
* tibble: Conflicts with tibble@1.0.0
This pair of packages are not installable together as there is an obvious conflict that tibble@1.0.0
does not meet version criteria within dplyr
. It's a significant limitation that we have to bear in mind if we want to keep it as a thin layer on pkgdepends
.
I have spent considerable amount of time of how to overcome this (and also fit nicely within the whole framework) with no success :( I can't really ask package maintainers for it as I am aware that this is a very specific requirement. This is true that currently named "min" deps is not a code-compabilitywise set of minimal dependencies - it's more of an installable (not to mention compileable) set of packages that is a bit more strict. I will emphasise this once more in the slides.
My view on this is that even if it's imperfect - this is a big step forward. I totally got your point about no-one asking for a specific very old version of a package. This is very unlikely to happen. I was thinking more about set of interdependent packages that are developed in parallel - something we are doing a lot. Always blindly vbumping deps is probably not the best strategy. On the other hand - keeping it unchanged is not correct either. Wanted to find a good tool that will warn you when vbump is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing our example I did a short experiment:
- lower the requirements on
tibble
from package description file - i.e.tibble (>= 2.1.3)
->tibble (>= 2.0.0)
- Remove dplyr and tibble packages
pak::pkg_install("dplyr@1.0.0")
pak::pkg_install("tibble@2.0.0")
- i.e. overinstall version 3.2.1 that has been installed withdplyr
- run rcmd build:
$> rcmdcheck::rcmdcheck("./insightsengineering/tern")
── R CMD build ────────────────────────────────────────────────────────────────
✔ checking for file ‘.../DESCRIPTION’ ...
─ preparing ‘tern’: (880ms)
✔ checking DESCRIPTION meta-information ...
─ cleaning src
─ installing the package to process help pages
-----------------------------------
─ installing *source* package ‘tern’
─ using staged installation
─ libs
Warning: no source files found
─ R
─ data
*** moving datasets to lazyload DB
─ inst
─ byte-compile and prepare package for lazy loading
Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
namespace ‘tibble’ 2.0.0 is already loaded, but >= 2.1.3 is required
Calls: <Anonymous> ... namespaceImportFrom -> asNamespace -> loadNamespace
Execution halted
ERROR: lazy loading failed for package ‘tern’
─ removing ‘/private/var/folders/m1/hrz0h_ls7gz57rc80tnj41t80000gp/T/RtmpmRIM0D/Rinst15a481d7e84a8/tern’
-----------------------------------
ERROR: package installation failed
Error in proc$get_built_file() : Build process failed
It seems that this is not possible to build your package if any of the direct dependencies cannot be loaded properly. It's not possible to have fully functional environment (on which you can validate your pkg compatibility) that is built using min versions specified in the DESCRIPTION file as is. Some adjustment needs to be done (here: tibble@2.0.0
-> tibble@2.1.3
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is... somewhat true, but backwards. The cascade goes the other way
- You cannot load your hard dependency (dplyr needs tibble >=2.1.3 and can't get it), so
- you cannot load your package, because you must load all hard dependencies to load your package
- you cannot install your package (because the installation process includes loading the package, which would fail even if you slipped past the other checks)
Note how I'm framing this. It has nothing to do with tibble per se. Its the dplyr requirement for our package that failed here. That's important.
Packages (which have non-base-R dependencies) can never be considered in isolation. Ever. For anything, in any circumstances.
But this is a deeper issue. There was no reason to force tibble 2.0.0 here when one of the active dependencies forced a higher version. The fact that that happened is a sign something in the algorithm isn't doing the right thing.
Put another way, if dplyr 1.0.0 requires 2.1.3 (and has declared as much), there was no reason to even try with tibble 2.0.0 and dplyr 1.0.0. It was literally impossible for that test to be successful a priori. Thus it was impossible for us to get any new information from running it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is a deeper issue. There was no reason to force tibble 2.0.0 here when one of the active dependencies forced a higher version. The fact that that happened is a sign something in the algorithm isn't doing the right thing.
It was just an experiment (not part of any algorihtm) of an environment that reflects 1:1 with what specified by direct dependencies.
Put another way, if dplyr 1.0.0 requires 2.1.3 (and has declared as much), there was no reason to even try with tibble 2.0.0 and dplyr 1.0.0. It was literally impossible for that test to be successful a priori. Thus it was impossible for us to get any new information from running it.
Yes exactly. That was the goal for it - to show that such environment is non-fuctional and malformed. This makes me think that such configuration of dependencies is not really testable hence: not correct? (don't know if "correct" is the right term here but I hope you got my point). You can't specify tibble@2.0.0
+ dplyr@1.0.0
as you are not able to validate its correctness.
Like I said - some adjustment (here: vbump tibble) needs to be done. In the current implementation it just errors. This is well embedded in the dependency resolution algorighm in pkgdepends. I am currently playing with my own resolution logic that uses min for direct and concurrent for indirect deps. It's definitely not super nice as I am relaying on private class fields but it's sort of working. Let me polish it and I will reach out to you again soon for feedback. But to be clear: that not means that I am convinced about correctness of dependencies specifications. It's just a way how to handle incorrect inputs and correct it on-the-fly.
inst/training_202307/202307.qmd
Outdated
|
||
- **min** | ||
|
||
Indirect dependencies are installed as usual using the newest available. Therefore: it is not guaranteed that the new release of indirect dependency will be fully compatible with some old version of direct dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This honestly is a pretty fatal flaw in the paradigm, as I mentioned above, but it is one that you can get around with some work that is already done in switchr.
inst/training_202307/202307.qmd
Outdated
|
||
- **max** | ||
|
||
This is the most instable environment changed by pushes to any of the dependencies. Also: the main branch is usually not guaranteed to be correct and you might pull broken package version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why GRAN only builds on version change. last version change is a bit more work (though switchr already knows how, so you can get it from that), but it's much much safer than HEAD of main
Hi @gmbecker I have pushed a new version that renames "min" into "min_direct" (to emphasise that only direct deps are searched for min version and indirect are installed as usual) plus also add two more strategies:
Both of them should be "stable" over time - unless it has a "@*release" dynamic pointer in any of tested package dependencies. I believe is that the "min_cohort" strategy is what you had in your mind. When testing it, I discovered a relatively big limitation - when there are many interdependent packages developed in parallel and if one of them needs a recent (e.g. development) version of another then the rest of dependency specification of that package is basically disregarded and the newest versions are used. Therefore we are not testing against min versions specified in DESC but rather against env determined by the most recent dependency out of all your dependencies. I am happy to hear your thoughts about that. We can even meet and discuss - that would be faster. Please ping me if you are ready. UPDATE: |
Please also find intermediate result created for min_cohort
min_cohorts
|
I have pushed a new commit that does the following:
So the current state is as follows
Both min strategies relies on CRAN PPM snapshots so as to limit the versions of indirect dependencies. The most "correct" strategy is the "min_cohort" where there is only one snapshot used determined by the most recently released direct dependent package. We take all the packages from the whole dependency tree (incl. indirect deps) using the greatest version as of certain date. PPM-based strategies makes dependency tree to be always resolvable and there is no need to break encapsulation and collapse the whole dependency tree into the tested package dependency specification. That was probably the biggest drawback of the "min_direct" strategy. Having said that, I think we need to acknowledge these limitations and move forward with what we have now. Of course we can improve as we go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more non-exhaustive comments
Remotes cleanup refactoring, code cleanup & bug fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good!
After this PR is merged 2 others should also be merged for the action to use the new strategies (and stops using min
)
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized before being ready to review. #### Change in code - [x] Change branch in verdepcheck-action from `new-strategies` back to `@main` #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Documentation update on strategy, replacing `min` with `min_isolate` and `min_cohort`
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #189 ### 🔴 Checklist for PR Reviewer - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #104 ### 🔴 Checklist for PR Reviewer - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #148 ### 🔴 Checklist for PR Reviewer - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #137 ### 🔴 Checklist for PR Reviewer - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * https://github.com/insightsengineering/teal.logger/pull/159 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: - #96 ### 🔴 Checklist for PR Reviewer ### 🔴 Checklist for PR Reviewer - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #163 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: - #54 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #527 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: - #278 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com> Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #130 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * [x] `formatters` release of the next version and update DESCRIPTION accordingly * `fmt_config` is required and only available at `formatters@main` atm * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #332 ### 🔴 Checklist for PR Reviewer [![Scheduled 🕰️](https://github.com/insightsengineering/teal.slice/actions/workflows/scheduled.yaml/badge.svg?branch=verdepcheck_action)](https://github.com/insightsengineering/teal.slice/actions/workflows/scheduled.yaml?query=branch%3Averdepcheck_action) _(see comment below)_ - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #45 ### 🔴 Checklist for PR Reviewer [![Scheduled 🕰️](https://github.com/insightsengineering/teal.logger/actions/workflows/scheduled.yaml/badge.svg?branch=verdepcheck_action)](https://github.com/insightsengineering/teal.logger/actions/workflows/scheduled.yaml?query=branch%3Averdepcheck_action) _(see comment below)_ - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com> Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 ### 🔴 Checklist for PR Reviewer [![Scheduled 🕰️](https://github.com/insightsengineering/tern.rbmi/actions/workflows/scheduled.yaml/badge.svg?branch=verdepcheck_action)](https://github.com/insightsengineering/tern.rbmi/actions/workflows/scheduled.yaml?query=branch%3Averdepcheck_action) - [x] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [x] Package versions are the same or higher than `main` - [x] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [x] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [x] Added entry to `NEWS.md` - [x] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [x] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #957 ### 🔴 Checklist for PR Reviewer [![Scheduled 🕰️](https://github.com/insightsengineering/tern/actions/workflows/scheduled.yaml/badge.svg?branch=verdepcheck_action)](https://github.com/insightsengineering/tern/actions/workflows/scheduled.yaml) _(~~max strategy fails due to tidyverse/ggplot2#5436 corrected upstream)_ - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 ### 🔴 Checklist for PR Reviewer [![Scheduled 🕰️](https://github.com/insightsengineering/teal/actions/workflows/scheduled.yaml/badge.svg?branch=verdepcheck_action)](https://github.com/insightsengineering/teal/actions/workflows/scheduled.yaml?query=branch%3Averdepcheck_action) _(`max` and `release` strategies are expected to fail... see below)_ - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - Only exception is `rmarkdown` (may have been removed on `Suggests`) - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: - #517 ### 🔴 Checklist for PR Reviewer - [ ] Tag yourself next to this repo on insightsengineering/nestdevs-tasks#7 - [ ] Package versions are the same or higher than `main` - [ ] Package list is the same - [ ] All packages in `Imports`, `Depends` & `Suggests` are in new section `Config/Needs/verdepcheck` - [ ] Added entry to `NEWS.md` - [ ] Last `scheduled.yaml` action was run succesfully _(all 4 strategies)_ - important: it's not the last commit, it's the one that runs 4 `Scheduled 🕰️ / Dependency` actions - [ ] `scheduled.yaml` SHOULD NOT have any push on any branches ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [ ] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [ ] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #780 ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [ ] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #206 ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [ ] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com> Co-authored-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
WIP :: parent issue: insightsengineering/nestdevs-tasks#7 Supersede: * #218 ### 🔴 What's needed before merging? This PR depends on some upstream changes that need to be finalized/merged before being ready to review. #### Change in code * `verdepcheck.yml` action (see comments) - [x] Remove `on: push` section - [x] Change branch to main #### PRS - [x] verdepcheck * insightsengineering/verdepcheck#24 * insightsengineering/verdepcheck#26 - [x] verdepcheck-action * insightsengineering/r-verdepcheck-action#16 ### Changes description * Adds minimum version for packages `DESCRIPTION` * Adds `Config/Need/verdepcheck` section in `DESCRIPTION` * Updates verdepcheck action --------- Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com> Co-authored-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
get_ref_min
andget_ref_min_incl_cran
config
arg to be a empty list to to avoid re-writting everything if one wants to change one option onlycli
install_ip
intoexecute_ip
and split it intosolve_ip
,download_ip
,install_ip
,check_ip
to better follow the logic (those are all private funs)