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

Use common renv.lock for all {admiral} packages #2

Closed
cicdguy opened this issue Aug 9, 2022 · 10 comments · Fixed by #16
Closed

Use common renv.lock for all {admiral} packages #2

cicdguy opened this issue Aug 9, 2022 · 10 comments · Fixed by #16

Comments

@cicdguy
Copy link
Collaborator

cicdguy commented Aug 9, 2022

Proposal

Use a common renv.lock file for all {admiral} packages on GitHub. Keep a centralized renv.lock file in this repository and add all dependencies from every other admiral extension here, and propagate it across all other repositories so that they share the same environment for testing, development, and release.

This is what the proposed workflow would look like this:

graph LR
    Developer --> |Manages fa:fa-lock renv.lock| A[fa:fa-box admiralci]
    A -->|fa:fa-lock renv.lock| C{fa:fa-link Propagator}
    C -->|fa:fa-arrow-right propagate| D[fa:fa-box admiralonco]
    C -->|fa:fa-arrow-right propagate| E[fa:fa-box  admiraldev]
    C -->|fa:fa-arrow-right propagate| F[fa:fa-box admiraltemplate]
    C -->|fa:fa-arrow-right propagate| G[fa:fa-box admiralophtha]
    C -->|fa:fa-arrow-right propagate| H[fa:fa-box admiral]
    C -->|fa:fa-arrow-right propagate| I[fa:fa-box admiral.test]
    C -->|fa:fa-arrow-right propagate| J[fa:fa-box admiralXXX]
    subgraph CI/CD
    C
    end
    subgraph Extensions
    D
    E
    F
    G
    H
    I
   J
    end
Loading

Propagation steps will be as follows:

  1. A developer creates a PR against this repo to update the renv.lock file. Let's refer to this PR as "central PR".
  2. The renv.lock file is validated for consistency and the standard CI/CD workflows are run against the "central PR".
  3. Simultaneously, additional PRs are created automatically (by a bot) on the {admiral} extensions which will validate the changes on the renv.lock file by running the standard CI/CD workflows that the extensions currently use. Let's refer to these PRs as "extension PRs".
  4. If everything looks good on the extensions, the "extension PRs" are merged first, and the "central PR" is merged last.
  5. If things go awry, troubleshooting issues is imperative and the corresponding updates are made to the central "central PR", which propagate updates to the renv.lock to the "extension PRs" and automatically run tests there.
  6. The previous step is repeated until all issues are resolved. Eventually, step 4 is executed.

Justification

  • As a user, if I wanted to install the entire family of admiral packages on a given system, variations in dependencies will cause dependency conflicts and installation issues.
  • Even if installations succeed, results could vary as various versions of dependencies could result in different computed values.
  • From an operation toil perspective, developers will only need to maintain and update the renv.lock file in one location, saving time and effort.
@cicdguy
Copy link
Collaborator Author

cicdguy commented Aug 9, 2022

CC @rossfarrugia @bundfussr @bms63 Please review the proposal.

@cicdguy cicdguy changed the title Use common renv.lock for all admiral packages Use common renv.lock for all {admiral} packages Aug 9, 2022
@rossfarrugia
Copy link
Collaborator

i like it Dinakar as discussed earlier. tagging @thomas-neitmann to double check too

@walkowif
Copy link
Collaborator

walkowif commented Aug 9, 2022

LGTM. The only thought I have is: is point 4. atomic (i.e. all PRs have to be merged immediately one after another)? If yes, what could make the order of merging extension PRs vs. central PR matter? Does the order of merging "extension PRs" matter?

@cicdguy
Copy link
Collaborator Author

cicdguy commented Aug 9, 2022

LGTM. The only thought I have is: is point 4. atomic (i.e. all PRs have to be merged immediately one after another)? If yes, what could make the order of merging extension PRs vs. central PR matter? Does the order of merging "extension PRs" matter?

My concern with doing the extension PRs first and then the central PR last is that there may or may not be some issues that we'll see post-merge on the extension repositories, which can be remediated on the central PR if we do find any issues. That way we're always putting in a battle-tested, high quality renv.lock file into this repository.

@walkowif
Copy link
Collaborator

walkowif commented Aug 9, 2022

Ok, will we check if anything went wrong after merging each extension PR, or after merging them all at once?
If they're merged all at once, then the central PR with remediation will have to open new extension PRs.
But if we merge one by one, central PR will have to update extension PRs in repos where they still haven't been merged, and open new extension PRs for repos that had their previous extension PRs merged.
But maybe that will become clearer during implementation.

@bms63
Copy link
Collaborator

bms63 commented Aug 9, 2022

Okay...how did you make that diagram...I want that power.

This all makes sense to me. We just need to be able to have a process, and make it well known, on how deverlopers can request updates to the renv.lock file. I think a lot of people think this thing is set in stone for eternity, but that is not the case.

@cicdguy
Copy link
Collaborator Author

cicdguy commented Aug 9, 2022

Okay...how did you make that diagram...I want that power.

GitHub now support mermaid.js: https://github.blog/2022-02-14-include-diagrams-markdown-files-mermaid/

This all makes sense to me. We just need to be able to have a process, and make it well known, on how deverlopers can request updates to the renv.lock file. I think a lot of people think this thing is set in stone for eternity, but that is not the case.

Agreed - this will need to be documented and well-described.

@thomas-neitmann
Copy link
Collaborator

Love thsi proposal (and the diagram)! Great suggestion @cicdguy 👏

I agree with @bms63 we'll have to create and document a process for how the central renv.lock file gets updated. SOunds like a new vignette for {admiraldev}.

@bms63
Copy link
Collaborator

bms63 commented Sep 27, 2022

Hey @cicdguy @walkowif Just seeing if this is completed? I saw a lot of movement around it last week and thought it might be done??

@cicdguy
Copy link
Collaborator Author

cicdguy commented Sep 27, 2022

This is almost done. We can finally create a new renv.lock file with an upgraded R version and propagate it across repos.

One final step remaining, however, is the need to update the R_VERSION in the workflows to match the R version used in the renv.lock file. I'll look into adding this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants