Refactor teal.gallery into a directory structure for individual shiny apps #31
Conversation
…g the .Rprofile manually
|
Abandoning this branch and PR in favor of the inst/apps structure. |
…rename quarto directory
vedhav
force-pushed
the
29_refactor_app_structure@main
branch
from
e6bc4c9
to
3b16e5c
Compare
.github/workflows/docs.yaml
Outdated
There was a problem hiding this comment.
as a next big step let's do the docs workflow. You will find a good example in tlg-catalog repo
There was a problem hiding this comment.
Added the CI for this and tested the deployment in fork: https://vedhav.github.io/teal.gallery
| sys.source(".Rprofile") | ||
| options(renv.config.cache.symlinks = FALSE) | ||
| options(renv.config.install.remotes = FALSE) | ||
| renv::restore(clean = TRUE) |
There was a problem hiding this comment.
this is quite lenghty command to complete. At the current moment it's 40+ mins and still counting... for multiple apps...
There was a problem hiding this comment.
Currently, the cache does not seem to work. But once the renv cache works this should not take too long for renv.lock that has no change. I haven't debugged the cause for this yet.
There was a problem hiding this comment.
In local once the packages are installed it should not take too long. I also made sure that other apps use the same version of the packages so there won't be too much waiting for the second app. This is quite natural for a large shiny app, As far as I know, there is not much we can do about it.
There was a problem hiding this comment.
So we have 9 apps checked on 2 platforms which gives us 18 workflows to be run. Each of them lasts aproximately ~1h (rounding up) which makes each commit "costs" us 18 hours of CI time. That's way to much.
This is quite natural for a large shiny app, As far as I know, there is not much we can do about it.
Yes that's true - that's why I am asking. Do we really want to trigger this on each commit? Maybe trigger it on changes in .lock files only? I was even thinking of abandoning this at all and leave it for deploy action to handle as deploy will have to anyway restore lock files.
There was a problem hiding this comment.
I really like the idea of triggering it on changes to the lock file. Ideally, the current CI file should be cached based on renv.lock hash but for some incorrect config the cache is not working.
I like abandoning this and merging this with the deploy CI. Additionally, we could benefit from running them in series as most of the package across the renv.lock are same.
Regardless we should fix the renv cache issue to speed up the CI runs.
There was a problem hiding this comment.
I suggest doing now the latter - i.e. remove it complitely and include that in the deploy CI
vedhav
force-pushed
the
29_refactor_app_structure@main
branch
from
5334a7f
to
bb04112
Compare
basic-teal/renv.lock
Outdated
There was a problem hiding this comment.
I am aware that this is a bigger topic outside of this PR but let me ask you to start thinking about it (but please don't do this here because we would never finish this PR lol).
This .lock file is a snapshot type of data that reflects the state as of certain point in time (most likely: the last update of this file). The whole ecosystem of R packages is moving quite fast and it's very likely that this information become outdated very soon. We need to have some autoupdate functionality to update refs in .lock file to the newest release of any of the dependencies. How to do that? I can't imagine this being a human responsibility so to me that should be handled in CI using bot commits. How to deal with dependencies coming from GH? I was thinking about having DESC field with Remotes section and get gh refs from there. I am open for ideas.
There was a problem hiding this comment.
Yeah, I agree that we should do it, and couldn't agree more that this must be automated.
I can see an additional thing we should do before we do the automatic update:
- Adding an end-to-end test that just visits every tab automatically and checks if there was any error.
Having an e2e test like that is the only way we can make sure that the updated packages do not break any app. This can also potentially be valuable to find some bugs with the integration of different nest packages. I know it can be done because I remember this was done with the help of {rhino} when we rewrote the FDA pilot (thanks to André who added this) https://github.com/Appsilon/rhino-fda-pilot/blob/main/tests/cypress/integration/app.spec.js
So, all we need to do is replicate that without {rhino}, generalise it and potentially add it in the https://github.com/insightsengineering/r.pkg.template/tree/main/.github/workflows
Closes #29 and #20