Skip to content

Conversation

@rubberduck203
Copy link
Contributor

@rubberduck203 rubberduck203 commented Jul 17, 2025

#208 fixed one caching bug by changing when we update the cache.
I'm unsure if that PR introduced this bug (it may have) but this PR fixes another.

When ever the same file path is used to cache on in subsequent actions, the cache is updated when the first succeeds, so the cache is read as valid for the second action when it should be invalid.

Example config file

apiVersion: scope.github.com/v1alpha
kind: ScopeDoctorGroup
metadata:
  name: ssh
  description: Setup SSH keys for GitHub authentication
spec:
  include: when-required
  needs: []
  actions:
    - name: private-key-exists
      description: Ensures SSH private key exists
      check:
        paths:
          - ~/.ssh/id_ed25519
        commands:
          - ./bin/ssh.sh check private-key
      fix:
        commands:
          - ./bin/ssh.sh fix private-key
    - name: public-key-exists
      description: Ensures SSH public key exists
      check:
        paths:
          - ~/.ssh/id_ed25519.pub
        commands:
          - ./bin/ssh.sh check public-key
      fix:
        commands:
          - ./bin/ssh.sh fix public-key
    - name: key-added
      description: Uploads the pub key to GitHub
      check:
        # The paths in this were previously erroneously cached too early
        # so the check was always passing
        paths:
          # if either of these files have changed, we need to re-check
          - ~/.ssh/id_ed25519
          - ~/.ssh/id_ed25519.pub
        commands:
          - ./bin/ssh.sh check github
      fix:
        commands:
          - ./bin/ssh.sh fix github

This because we were keying only off the group name and the fully qualified file name.

This PR updates the cache to also key off of the action name, so that each check has a unique key in the cache.

Old cache format

{
  "checksums": {
    "rails-group": {
      "/path/to/file1.txt": "abc123",
      "/path/to/file2.txt": "def456"
    },
    "ruby-group": {
      "/path/to/ruby-version": "xyz789"
    }
  }
}

New cache format

{
  "checksums": {
    "rails-group": {
        "action-1": { "/path/to/file1.txt": "abc123" },
        "action-2": { "/path/to/file2.txt": "def456" }
    },
    "ruby-group": {
      "action-3": { "/path/to/ruby-version": "xyz789" }
    }
  }
}

IMPORTANT: This will invalidate any existing cache on a user's machine. The first time a user runs with this new version, we will invalidate all of their existing caches.

@rubberduck203 rubberduck203 force-pushed the cmac/cache-at-action-level branch from 87ed702 to e609e2b Compare July 17, 2025 18:04
@rubberduck203 rubberduck203 requested a review from Copilot July 17, 2025 18:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the file caching system to cache at the action level instead of the group level, providing more granular cache management. The change allows different actions within the same group to maintain separate cache entries for the same files.

  • Updates the FileCache trait to accept both group and action names for cache operations
  • Restructures the cache data format to use a three-level hierarchy: group → action → file
  • Adds comprehensive test coverage for the new caching behavior and backward compatibility

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
scope/src/doctor/file_cache.rs Refactors cache data structures and trait methods to support action-level caching, adds extensive test suite
scope/src/doctor/check.rs Updates method calls to pass both group and action names to the file cache
scope/Cargo.toml Adds tempfile dependency for testing
Comments suppressed due to low confidence (1)

scope/src/doctor/file_cache.rs:583

  • This test attempts to create a cache at the root path ('/') but this behavior is platform-dependent and may not fail on all systems. Consider using a more reliable invalid path or mocking the file system operations for consistent test behavior.
        async fn test_persist_invalid_path() {

@rubberduck203 rubberduck203 force-pushed the cmac/cache-at-action-level branch from e609e2b to 6d77783 Compare July 17, 2025 19:30
@rubberduck203 rubberduck203 marked this pull request as ready for review July 17, 2025 20:05
@rubberduck203 rubberduck203 merged commit 7a230e3 into Gusto:main Jul 18, 2025
13 checks passed
@rubberduck203 rubberduck203 deleted the cmac/cache-at-action-level branch July 18, 2025 01:58
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.

2 participants