Skip to content

CI: support run docker build/test on PR - #1408

Merged
tuhaihe merged 2 commits into
apache:mainfrom
tuhaihe:docker-build-event
Oct 29, 2025
Merged

CI: support run docker build/test on PR#1408
tuhaihe merged 2 commits into
apache:mainfrom
tuhaihe:docker-build-event

Conversation

@tuhaihe

Copy link
Copy Markdown
Member

Enable both docker-cbdb-build-containers and docker-cbdb-test-containers workflows to run on pull_request when files under their respective Docker paths change.

Main changes:

  • Add pull_request triggers with path filters:
  • build: devops/deploy/docker/build/**
  • test: devops/deploy/docker/test/**

Guard Docker hub login and multi-arch push steps so they run only push to refs/heads/main.

Fixes #ISSUE_Number

What does this PR do?

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Breaking Changes

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions


@leborchukleborchuk self-assigned this Oct 20, 2025
Comment thread.github/workflows/docker-cbdb-test-containers.yml
@edespino
edespino self-requested a review October 21, 2025 21:23

@edespinoedespino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR Feedback: Enhancement Suggestion for User Experience

PR #1408: CI: support run docker build/test on PR

Suggestion: Add PR Context to Build Summary

The current implementation correctly guards Docker Hub operations, but contributors may not immediately understand why certain steps are skipped
when viewing PR checks.

Proposed Enhancement

Add a conditional PR context section to the Build Summary step that appears only on PR builds:

For .github/workflows/docker-cbdb-build-containers.yml (line ~192):

# Add PR context notification
if [[ "${{ github.event_name }}" == "pull_request" ]]; then
echo "#### ℹ️ Pull Request Build" >> $GITHUB_STEP_SUMMARY
echo "This is a validation build. Images are built and tested locally but **not pushed to Docker Hub** for security." >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "- ✅ Dockerfile syntax validated" >> $GITHUB_STEP_SUMMARY
echo "- ✅ Multi-architecture builds tested" >> $GITHUB_STEP_SUMMARY
echo "- ✅ TestInfra tests executed" >> $GITHUB_STEP_SUMMARY
echo "- ⏭️ Docker Hub push skipped (requires main branch)" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
fi

For .github/workflows/docker-cbdb-test-containers.yml (line ~166):

# Add PR context notification
if [[ "${{ github.event_name }}" == "pull_request" ]]; then
echo "#### ℹ️ Pull Request Build" >> $GITHUB_STEP_SUMMARY
echo "This is a validation build. Images are built and tested locally but **not pushed to Docker Hub** for security." >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "- ✅ Dockerfile syntax validated" >> $GITHUB_STEP_SUMMARY
echo "- ✅ Multi-architecture builds tested" >> $GITHUB_STEP_SUMMARY
echo "- ⏭️ Docker Hub push skipped (requires main branch)" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
fi

Benefits

  • Clarity: Contributors immediately understand the security model
  • Transparency: Makes the workflow behavior explicit in the UI
  • Consistency: Matches the pattern already used elsewhere in the codebase (test workflow line 98-100)

Validation

Tested with stub workflow in isolated repository - confirmed the conditional logic works correctly and displays as expected in GitHub Actions UI.

Note: This is a nice-to-have enhancement for user experience, not required for functionality.

Enable both docker-cbdb-build-containers and docker-cbdb-test-containers
workflows to run on pull_request when files under their respective Docker
paths change.
Main changes:
* Add pull_request triggers with path filters:
- build: devops/deploy/docker/build/**
- test: devops/deploy/docker/test/**
Guard Docker hub login and multi-arch push steps so they run only push
to refs/heads/main.
@tuhaihe

Copy link
Copy Markdown
MemberAuthor

PR Feedback: Enhancement Suggestion for User Experience

PR #1408: CI: support run docker build/test on PR

Suggestion: Add PR Context to Build Summary

The current implementation correctly guards Docker Hub operations, but contributors may not immediately understand why certain steps are skipped when viewing PR checks.

Proposed Enhancement

Add a conditional PR context section to the Build Summary step that appears only on PR builds:

Great feedback! I addressed this feedback at the latest commit 513b0ef. FYI.

@tuhaihe
tuhaihe merged commit 9b84324 into apache:mainOct 29, 2025
28 checks passed
Sign up for freeto 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.

3 participants

@tuhaihe@edespino@leborchuk