Skip to content

Conversation

@bromiesTM
Copy link

@bromiesTM bromiesTM commented Dec 10, 2025

  • Change jfrog path to a non 'poc' path once review ok

@bromiesTM bromiesTM force-pushed the kh/dev/cache-build-pipeline branch 15 times, most recently from 57530ae to 672d39e Compare December 15, 2025 12:05
@bromiesTM bromiesTM requested a review from Copilot December 15, 2025 12:06
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 optimizes the Nextcloud Workspace build pipeline by implementing intelligent caching using GitHub Actions cache and JFrog Artifactory. Instead of rebuilding all apps on every run, the workflow now checks for cached build artifacts based on git submodule SHAs and only builds apps that have changed or lack cached artifacts. The optimization includes retry logic for JFrog uploads/triggers, workflow dispatch with force rebuild capability, and comprehensive cache status reporting.

Key Changes

  • Implemented cache detection mechanism to identify which apps need building vs. can be restored from cache (GitHub Actions cache or JFrog)
  • Added retry logic with exponential backoff for JFrog uploads and GitLab pipeline triggers
  • Added workflow dispatch with force rebuild option to bypass all caches when needed

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
.github/workflows/build-artifact.yml Complete workflow refactoring with cache-aware building, conditional job execution, app restoration from JFrog/cache, retry logic, and force rebuild support
.github/scripts/detect-app-cache.sh New script implementing cache detection logic that checks GitHub cache and JFrog for existing app builds and determines which apps need building

Comment on lines 425 to 439
# Use actions/cache/restore in a way that works in a script context
# We need to use gh CLI to restore the cache
if gh cache restore "$CACHE_KEY" --key "$CACHE_KEY"; then
echo "✅ Restored $APP_NAME from GitHub cache"
# Validate restoration
if [ ! -d "$APP_PATH" ] || [ ! -f "$APP_PATH/appinfo/info.xml" ]; then
echo "❌ Validation failed for $APP_NAME"
exit 1
fi
else
echo "❌ Failed to restore from GitHub cache"
exit 1
fi
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The gh cache restore command does not exist in the GitHub CLI. Use the actions/cache/restore action instead, or implement a workaround since this is within a run script. Consider restructuring this as a separate step using the cache action.

Suggested change
# Use actions/cache/restore in a way that works in a script context
# We need to use gh CLI to restore the cache
if gh cache restore "$CACHE_KEY" --key "$CACHE_KEY"; then
echo "✅ Restored $APP_NAME from GitHub cache"
# Validate restoration
if [ ! -d "$APP_PATH" ] || [ ! -f "$APP_PATH/appinfo/info.xml" ]; then
echo "❌ Validation failed for $APP_NAME"
exit 1
fi
else
echo "❌ Failed to restore from GitHub cache"
exit 1
fi
# NOTE: Cannot restore GitHub cache from within a script.
# Use the actions/cache/restore action as a separate step/job for each app.
echo "❌ ERROR: Cannot restore GitHub cache from within a script. Please use actions/cache/restore in a separate step."
exit 1

Copilot uses AI. Check for mistakes.
if [ $ATTEMPT -lt $MAX_ATTEMPTS ]; then
echo "Waiting $DELAY_SEC seconds before retry..."
sleep $DELAY_SEC
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: delays are 10s, then 20s (sleep occurs after failed attempts)
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The comment states delays are 10s then 20s, but the logic produces delays of 10s, 20s, and 40s across three attempts. The comment should be updated to accurately reflect the actual exponential backoff pattern: '10s, 20s, 40s'.

Suggested change
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: delays are 10s, then 20s (sleep occurs after failed attempts)
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: delays are 10s, 20s, 40s (sleep occurs after failed attempts)

Copilot uses AI. Check for mistakes.
if [ $ATTEMPT -lt $MAX_ATTEMPTS ]; then
echo "Waiting ${DELAY_SEC} seconds before retry..."
sleep $DELAY_SEC
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: 5s, 10s, 20s
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The comment states delays are 5s, 10s, 20s, but with three attempts the actual delays between retries are only 5s and 10s (two retries after the first failure). The comment should clarify that these are the delays between the 3 attempts, not 4 attempts.

Suggested change
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: 5s, 10s, 20s
DELAY_SEC=$((DELAY_SEC * 2)) # Exponential backoff: delays between attempts are 5s, 10s (for 3 attempts)

Copilot uses AI. Check for mistakes.
fi

# Check if cache exists using GitHub CLI
# Include --ref to access caches from the current ref (branch, PR, etc.)
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

Using --key as a filter in gh cache list requires an exact match, but the intention seems to be finding a cache with this specific key. Consider using --key as a prefix matcher or verify that the exact key matching is intentional and document this behavior.

Suggested change
# Include --ref to access caches from the current ref (branch, PR, etc.)
# Include --ref to access caches from the current ref (branch, PR, etc.)
# NOTE: We intentionally use --key for exact cache key matching.
# Only exact matches are considered valid for restore; prefix fallback is not used.

Copilot uses AI. Check for mistakes.
@bromiesTM bromiesTM marked this pull request as ready for review December 15, 2025 12:09
Copy link

@seriAlizations seriAlizations left a comment

Choose a reason for hiding this comment

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

LGTM but please fix spelling errors

@bromiesTM bromiesTM force-pushed the kh/dev/cache-build-pipeline branch 7 times, most recently from a676a5b to 22c1a0b Compare December 17, 2025 10:22
for later performance comparison

Signed-off-by: Misha M.-Kupriyanov <[email protected]>
in order not to overwrite optimized artifact

Signed-off-by: Misha M.-Kupriyanov <[email protected]>
printminion-co and others added 12 commits December 17, 2025 11:38
drop validation since it is now Redundant (by design)

Signed-off-by: Misha M.-Kupriyanov <[email protected]>
Optimized build workflow that uses cache-based detection
 - Checks cache for each app's current SHA
 - Only builds apps with no cached build
 - Significantly reduces build time through smart caching

Signed-off-by: Misha M.-Kupriyanov <[email protected]>
Implement intelligent cache detection for Nextcloud apps in the build
artifact workflow. This enhancement improves build performance by
detecting which apps need rebuilding based on git changes.

Key features:
- Detect changed apps using git diff against base branch
- Add verbose logging for debugging cache behavior
- Optimize performance with parallel cache operations
- Remove strict PR requirement to enable testing flexibility

Signed-off-by: Kai Henseler <[email protected]>
Extract cache detection logic from workflow YAML into a dedicated
script file for better maintainability and testability.

Changes:
- Create `.github/scripts/detect-app-cache.sh` with all detection logic
- Simplify workflow file by calling the external script
- Improve code readability and separation of concerns
- Enable easier testing and iteration of cache detection logic

Signed-off-by: Kai Henseler <[email protected]>
@bromiesTM bromiesTM force-pushed the kh/dev/cache-build-pipeline branch from 22c1a0b to ee46a9f Compare December 17, 2025 10:38
@bromiesTM bromiesTM merged commit 59b898c into ionos-donot-merge Dec 17, 2025
4 of 6 checks passed
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.

4 participants