ci: add environment declaration for DockerhHub publish #78
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Warning Rate limit exceeded@aaronsteers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/pypi_publish.yml (4)
Line range hint
73-85: How about adding version format validation? 🤔While the version setting logic works, we might want to add explicit version format validation to catch issues early. Something like this perhaps? wdyt?
- name: Set Version (workflow_dispatch) if: github.event_name == 'workflow_dispatch' run: | + if ! [[ "${{ github.event.inputs.version }}" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9]+)?$ ]]; then + echo "Error: Version must follow semantic versioning (e.g., 1.0.0 or 1.0.0-dev1)" + exit 1 + fi echo "Version set to ${{ github.event.inputs.version }}" echo "VERSION=${{ github.event.inputs.version }}" >> $GITHUB_ENV
Line range hint
91-95: Consider pinning Docker actions with SHA digests for enhanced security? 🔒While using version tags is good, using SHA digests provides even better security guarantees. Here's a suggestion, wdyt?
- uses: docker/setup-qemu-action@v3 - uses: docker/setup-buildx-action@v3 + uses: docker/setup-qemu-action@e81a89b1732b9c48f2f8c04157c47e07426b29e6 # v3 + uses: docker/setup-buildx-action@f03ac48505955848960e80bbb68046aa35c7b9e7 # v3
Line range hint
102-115: How about enhancing the tag check error handling? 🤔The tag existence check is great! Would you consider making it more robust with better error handling? Something like:
- name: Check for existing tag run: | tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" if [ -z "$tag" ]; then echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." exit 1 fi echo "Checking if tag '$tag' exists on DockerHub..." - if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then + if ! DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then + echo "No existing tag '$tag' found. Proceeding with publish." + else echo "The tag '$tag' already exists on DockerHub. Skipping publish to prevent overwrite." + echo "If you need to update this tag, please delete it first from DockerHub." exit 1 fi - echo "No existing tag '$tag' found. Proceeding with publish."
Line range hint
117-126: Would you like to speed up builds with Docker layer caching? 🚀The build configuration looks solid! For faster builds, especially useful in development, we could add layer caching. Here's a suggestion, wdyt?
- name: Build and push uses: docker/build-push-action@v5 with: context: . platforms: linux/amd64,linux/arm64 push: true + cache-from: type=gha + cache-to: type=gha,mode=max tags: | airbyte/source-declarative-manifest:latest airbyte/source-declarative-manifest:${{ env.VERSION }} airbyte/source-declarative-manifest:${{ github.sha }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/pypi_publish.yml (1)
68-70: LGTM! Good security practice with environment declaration! 🔒
The environment declaration for DockerHub is a great security practice, making the deployment target explicit and enabling environment-specific protection rules.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.github/workflows/pypi_publish.yml (4)
1-4: Consider enhancing the documentation clarityThe comments could be more specific about when publishing actually occurs. Maybe something like: "Builds on all pushes, but only publishes to PyPI and DockerHub on release tags or manual workflow dispatch." wdyt? 🤔
68-70: Consider using a more specific DockerHub URLThe current URL points to the tags page. Would it be more helpful to use the main repository URL instead? Something like:
https://hub.docker.com/r/airbyte/source-declarative-manifest? This would give a better overview of the project. wdyt? 🤔
Line range hint
116-129: Consider enhancing the tag check robustnessThe tag check is a great safety measure! 🛡️ Would you consider making it more robust with these additions?:
- name: Check for existing tag run: | tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" - if [ -z "$tag" ]; then + if [ -z "${{ env.VERSION }}" ]; then echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." exit 1 fi echo "Checking if tag '$tag' exists on DockerHub..." + # Ensure docker CLI is available + if ! command -v docker &> /dev/null; then + echo "Error: docker CLI is not available" + exit 1 + fi if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then echo "The tag '$tag' already exists on DockerHub. Skipping publish." exit 1 fi echo "No existing tag '$tag' found. Proceeding with publish."This adds:
- More accurate VERSION check
- Docker CLI availability check
wdyt? 🤔
Line range hint
102-108: Consider using SHA digests for Docker actionsFor enhanced security, would you consider using SHA digests instead of version tags for Docker-related actions? For example:
- uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@e81a89b1732b9c4836a5847f0f4c0fb8426b2d50 # v3 - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@f03ac48505955848960e80bbb68046aa35c7b9e7 # v3This prevents supply chain attacks by ensuring we use specific, verified versions. What are your thoughts on this? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(3 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/pypi_publish.yml
87-87: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pypi_publish.yml (2)
Line range hint
115-127: Strengthen the tag check logic? 🤔The tag check is great! Would you consider making it more robust by:
- Adding quotes around the
$tagvariable to handle special characters?- Using a more specific error code for tag existence (currently using 1 for both errors)?
Here's a possible improvement, wdyt?
- name: Check for existing tag run: | - tag="airbyte/source-declarative-manifest:${{ env.VERSION }}" + tag="airbyte/source-declarative-manifest:${VERSION:?No version specified}" if [ -z "$tag" ]; then echo "Error: VERSION is not set. Ensure the tag follows the format 'refs/tags/vX.Y.Z'." - exit 1 + exit 2 fi echo "Checking if tag '$tag' exists on DockerHub..." if DOCKER_CLI_EXPERIMENTAL=enabled docker manifest inspect "$tag" > /dev/null 2>&1; then echo "The tag '$tag' already exists on DockerHub. Skipping publish to prevent overwrite." - exit 1 + exit 3 fi echo "No existing tag '$tag' found. Proceeding with publish."
Line range hint
128-140: Consider adding security scanning? 🔒The Docker build setup looks solid! Would you be interested in adding a vulnerability scan step before pushing? We could use something like:
- Trivy for container scanning
- Docker Scout for supply chain security
Here's a quick example of how we could add Trivy, wdyt?
- name: Scan image for vulnerabilities uses: aquasecurity/trivy-action@master with: image: airbyte/source-declarative-manifest:${{ env.VERSION }} format: 'table' exit-code: '1' ignore-unfixed: true severity: 'CRITICAL,HIGH'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pypi_publish.yml(3 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/pypi_publish.yml
89-89: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/pypi_publish.yml (2)
68-70: Nice environment declaration! 👍
Clean and well-structured environment configuration with a helpful URL reference.
79-85: Consolidate version handling logic
Also applies to: 89-95
🧰 Tools
🪛 actionlint
79-79: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
…erHub
Summary by CodeRabbit