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

[Docker, CI] Allow building pecan/depends from pecan/depends #3260

Merged
merged 19 commits into from
May 16, 2024

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Feb 15, 2024

Description

For discussion -- definitely needs input from @robkooper before merging!

Adds a third option for the dependency image when building Docker images: In addition to "use the existing image" and "build from scratch", adds "try to update an existing image".

Also edits the Docker CI job to check for changed dependencies when building Docker images in a PR branch, and if detected to update the depends image before building other images on top of it.

Additionally and unrelatedly, adds workflow_dispatch hooks to several more workflows for easier debugging.

Motivation and Context

#3237 is currently failing the Docker CI check because it introduces new version requirements on dplyr and dbplyr that will be satisfied at Depends build time but that Make doesn't know how to resolve. Trying to fix this in the Makefile would add a lot of only-rarely-used complexity, and pre-emptively rebuilding the depends image from scratch on every CI run would be slow and wasteful.

Running the depends build only when it will actually change anything (i.e. when the PR touches docker/depends/*) is a first step, but it still feels wasteful to wait for all hojillion deps to reinstall when we really only added one or two. Then I realized that the two time-consuming steps in the depends build (apt updates and R package installation) will both skip packages that are already present on the disk -- so if we start from an existing pecan/depends image, adding a layer with the changed dependencies should be fast.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@infotroph
Copy link
Member Author

@robkooper Discussion questions:

  • Do you agree we don't want to push images built with this approach back to Docker hub? The point here is to unwedge CI statuses so that we can merge with confidence; better for the pushed images to spend the time for a full clean build.
  • If yes, do we need any explicit safeguards to prevent such pushing? I think that as currently implemented they won't be pushed because the update-build happens only on pull requests (technically only when github.base_ref is set, but it's documented to only be set for pull requests), but maybe that's too indirect to rely on as a safeguard.

@infotroph infotroph marked this pull request as ready for review March 4, 2024 20:54
@infotroph
Copy link
Member Author

/style

docker/depends/Dockerfile Show resolved Hide resolved
@infotroph infotroph added this pull request to the merge queue May 16, 2024
Merged via the queue into PecanProject:develop with commit cd01455 May 16, 2024
13 checks passed
@infotroph infotroph deleted the allow-depends-update branch May 18, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants