-
Notifications
You must be signed in to change notification settings - Fork 126
draft: modularize workflow and test #1613
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
base: main
Are you sure you want to change the base?
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Pull Request Test Coverage Report for Build 19453083700Details
💛 - Coveralls |
There was a problem hiding this 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 draft PR modularizes the CI/CD workflows by introducing reusable composite actions and workflow templates to reduce duplication and improve maintainability across the repository's GitHub Actions infrastructure.
Key Changes:
- Introduced 5 composite actions for common tasks (setup-operator-tools, setup-k8s-tools, configure-cloud-auth, collect-test-artifacts, build-and-sign-image)
- Added 4 reusable workflows for integration testing, image building/verification, and composite action testing
- Updated Go dependencies (Ginkgo v2.23.4→v2.27.2, Gomega v1.38.0→v1.38.2, go-logr v1.4.2→v1.4.3, and various golang.org/x packages)
- Reordered imports in 8 test files to follow Go conventions (dot imports after regular imports)
Reviewed Changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/test-composite-actions.yml | New workflow to test composite actions on the workflow-fix branch |
| .github/workflows/reusable-verify-image.yml | Reusable workflow for verifying container image signatures using cosign |
| .github/workflows/reusable-integration-test.yml | Reusable workflow template for running integration tests across cloud platforms |
| .github/workflows/reusable-build-operator.yml | Reusable workflow for building and scanning operator images with Trivy |
| .github/workflows/modernized-build-test-push.yml | Modernized build/test workflow leveraging new composite actions and reusable workflows |
| .github/actions/setup-operator-tools/action.yml | Composite action for installing Operator SDK and Ginkgo |
| .github/actions/setup-k8s-tools/action.yml | Composite action for installing kubectl, helm, eksctl, and k8s add-ons |
| .github/actions/configure-cloud-auth/action.yml | Composite action for multi-cloud authentication (AWS/Azure/GCP) |
| .github/actions/collect-test-artifacts/action.yml | Composite action for collecting and uploading test logs and artifacts |
| .github/actions/build-and-sign-image/action.yml | Composite action for building container images and signing with cosign |
| go.mod | Updated direct dependencies (Ginkgo, Gomega, go-logr) and added/removed indirect dependencies |
| go.sum | Synchronized checksums for updated and new indirect dependencies |
| test/smoke/smoke_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/smartstore/smartstore_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/smartstore/manager_smartstore_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/secret/secret_s1_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/secret/secret_m4_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/secret/secret_c3_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/secret/manager_secret_s1_test.go | Reordered imports (moved types import after ginkgo dot import) |
| test/secret/manager_secret_m4_test.go | Reordered imports (moved types import after ginkgo dot import) |
| cmd/main.go | Removed trailing whitespace from comment and fixed indentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - name: Set up Cosign | ||
| if: inputs.sign-image == 'true' | ||
| uses: sigstore/cosign-installer@main |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using @main for the sigstore/cosign-installer action is not recommended for production workflows as it can introduce breaking changes without notice. Pin to a specific version or use a commit SHA for stability and security.
Example:
uses: sigstore/[email protected]| uses: sigstore/cosign-installer@main | |
| uses: sigstore/cosign-installer@v3.5.0 |
| if: inputs.eksctl-version != '' | ||
| shell: bash | ||
| run: | | ||
| curl --silent --insecure --location "https://github.com/weaveworks/eksctl/releases/download/${{ inputs.eksctl-version }}/eksctl_$(uname -s)_amd64.tar.gz" | tar xz -C /tmp |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The insecure flag --insecure is used when downloading eksctl. This disables certificate verification and could allow man-in-the-middle attacks. Remove the --insecure flag to ensure secure downloads.
Change:
curl --silent --location "https://github.com/weaveworks/eksctl/releases/download/${{ inputs.eksctl-version }}/eksctl_$(uname -s)_amd64.tar.gz" | tar xz -C /tmp| curl --silent --insecure --location "https://github.com/weaveworks/eksctl/releases/download/${{ inputs.eksctl-version }}/eksctl_$(uname -s)_amd64.tar.gz" | tar xz -C /tmp | |
| curl --silent --location "https://github.com/weaveworks/eksctl/releases/download/${{ inputs.eksctl-version }}/eksctl_$(uname -s)_amd64.tar.gz" | tar xz -C /tmp |
| TEST_CLUSTER_PLATFORM: ${{ inputs.cluster-platform }} | ||
| TEST_CLUSTER_NAME: ${{ inputs.cluster-name }} | ||
| ECR_REPOSITORY: ${{ secrets.ECR_REPOSITORY }} | ||
| ECR_REGISTRY: ${{ secrets.ECR_REPOSITORY }} |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The environment variable ECR_REGISTRY is set to the same value as ECR_REPOSITORY (line 96), but ECR_REGISTRY doesn't appear to be used anywhere in the workflow. Consider removing this redundant variable or clarifying its purpose if it's used by the make commands.
| ECR_REGISTRY: ${{ secrets.ECR_REPOSITORY }} |
| branches: | ||
| - main | ||
| - develop | ||
| - workflow-fix # Added for testing |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment on line 12 mentions "Added for testing" which suggests this is a temporary addition. Ensure this comment is removed or updated before merging to clarify whether workflow-fix is a permanent supported branch.
| - workflow-fix # Added for testing | |
| - workflow-fix |
| branches: | ||
| - workflow-fix |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow triggers on the workflow-fix branch which appears to be a temporary test branch. This should be removed before merging to ensure the workflow doesn't run on unintended branches in production.
| branches: | |
| - workflow-fix | |
| # branches: | |
| # - workflow-fix |
| outputs: | ||
| image-digest: | ||
| description: 'Image digest SHA' | ||
| value: ${{ steps.build.outputs.digest }} |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outputs section is defined but there's no step with id: build that sets the digest output. The build step doesn't have an id, so ${{ steps.build.outputs.digest }} will always be empty. Either add an id: build to the "Build and Push Image" step and ensure it outputs the digest, or remove the unused outputs section.
| - name: Install Operator SDK | ||
| shell: bash | ||
| run: | | ||
| export ARCH=$(case $(uname -m) in x86_64) echo -n amd64 ;; aarch64) echo -n arm64 ;; *) echo -n $(uname -m) ;; esac) |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The script uses inline shell evaluation with $(case ...) which can be fragile. While functional, this could be simplified for better readability:
ARCH=$(uname -m)
case $ARCH in
x86_64) ARCH=amd64 ;;
aarch64) ARCH=arm64 ;;
esac
export ARCH
export OS=$(uname | awk '{print tolower($0)}')| export ARCH=$(case $(uname -m) in x86_64) echo -n amd64 ;; aarch64) echo -n arm64 ;; *) echo -n $(uname -m) ;; esac) | |
| ARCH=$(uname -m) | |
| case "$ARCH" in | |
| x86_64) ARCH=amd64 ;; | |
| aarch64) ARCH=arm64 ;; | |
| esac | |
| export ARCH |
| aws sts get-caller-identity | ||
| echo "" | ||
| echo "✅ configure-cloud-auth composite action works for AWS!" |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling for the aws sts get-caller-identity command is missing. If AWS authentication fails, the workflow should fail rather than silently continue. Add error handling:
if aws sts get-caller-identity; then
echo "✅ configure-cloud-auth composite action works for AWS!"
else
echo "❌ AWS authentication failed!"
exit 1
fi| aws sts get-caller-identity | |
| echo "" | |
| echo "✅ configure-cloud-auth composite action works for AWS!" | |
| if aws sts get-caller-identity; then | |
| echo "" | |
| echo "✅ configure-cloud-auth composite action works for AWS!" | |
| else | |
| echo "❌ AWS authentication failed!" | |
| exit 1 | |
| fi |
| TEST_BUCKET: ${{ secrets.TEST_BUCKET }} | ||
| TEST_INDEXES_S3_BUCKET: ${{ secrets.TEST_INDEXES_S3_BUCKET }} | ||
| CLUSTER_WIDE: ${{ inputs.cluster-wide }} | ||
| DEPLOYMENT_TYPE: ${{ inputs.deployment-type }} |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The deployment-type input is stored in the DEPLOYMENT_TYPE environment variable but is never used in the workflow steps. Either remove this unused input/environment variable or add documentation explaining when it will be used.
| DEPLOYMENT_TYPE: ${{ inputs.deployment-type }} |
|
|
||
| - name: Run Code Coverage | ||
| run: goveralls -coverprofile=coverage.out -service=circle-ci -repotoken ${{ secrets.COVERALLS_TOKEN }} |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow uses ${{ secrets.COVERALLS_TOKEN }} but this secret is not documented or validated. If the secret is not set, goveralls will fail silently or with unclear errors. Consider adding a check to verify the secret exists before using it, or document that this secret is optional for non-coverage runs.
| - name: Run Code Coverage | |
| run: goveralls -coverprofile=coverage.out -service=circle-ci -repotoken ${{ secrets.COVERALLS_TOKEN }} | |
| # This step uploads coverage to Coveralls. It requires the COVERALLS_TOKEN secret to be set. | |
| - name: Run Code Coverage | |
| if: ${{ secrets.COVERALLS_TOKEN != '' }} | |
| run: goveralls -coverprofile=coverage.out -service=circle-ci -repotoken ${{ secrets.COVERALLS_TOKEN }} | |
| # If the secret is not set, this step will be skipped. See documentation for details. |
Description
What does this PR have in it?
Key Changes
Highlight the updates in specific files
Testing and Verification
How did you test these changes? What automated tests are added?
Related Issues
Jira tickets, GitHub issues, Support tickets...
PR Checklist