test branch #983
test branch #983
Conversation
Korjen97
requested review from
AlvoBen,
greensd4 and
miryamfoiferCX
as code owners
|
New Issues (54)Checkmarx found the following issues in this Pull Request
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
.github/workflows/ci.yml
Outdated
| @@ -14,6 +14,7 @@ jobs: | |||
| with: | |||
| go-version-file: go.mod | |||
| - run: go version | |||
| - run: docker version | |||
There was a problem hiding this comment.
The addition of docker version command seems unrelated to the rest of the workflow. Please provide context for why this is necessary, and ensure it aligns with the workflow's purpose.
internal/commands/scan.go
Outdated
| @@ -60,7 +60,7 @@ const ( | |||
| containerVolumeFlag = "-v" | |||
| containerNameFlag = "--name" | |||
| containerRemove = "--rm" | |||
| containerImage = "checkmarx/kics:latest" | |||
| containerImage = "checkmarx/kics:2.1.3" | |||
There was a problem hiding this comment.
It's not recommended to replace a 'latest' tag with a specific version in a codebase without a clear rationale. This change could lead to the use of outdated images if not maintained properly. Please ensure that there's a specific reason for using version 2.1.3 and document the reason for this change, or consider using a strategy to keep up to date with the latest version.
| @@ -60,7 +60,7 @@ const ( | |||
| containerVolumeFlag = "-v" | |||
| containerNameFlag = "--name" | |||
| containerRemove = "--rm" | |||
| containerImage = "checkmarx/kics:latest" | |||
| containerImage = "checkmarx/kics:v2.1.3" | |||
There was a problem hiding this comment.
The container image tag has been changed to include a v prefix. Ensure that the tag v2.1.3 exists in the container registry and is the correct version intended for use. If this is a new tagging convention, update any documentation or scripts that reference the old tag format to prevent confusion.
.github/workflows/ci.yml
Outdated
| @@ -5,7 +5,9 @@ on: | |||
|
|
|||
| jobs: | |||
| unit-tests: | |||
| runs-on: ubuntu-latest | |||
| runs-on: ubuntu-24.04 | |||
There was a problem hiding this comment.
The runs-on value ubuntu-24.04 may not be a valid GitHub Actions runner label. GitHub Actions typically uses the ubuntu-latest or specific LTS versions like ubuntu-20.04. Please ensure that the runner label is correct and supported by GitHub Actions.
.github/workflows/ci.yml
Outdated
| runs-on: ubuntu-latest | ||
| runs-on: ubuntu-24.04 | ||
| container: | ||
| image: ubuntu-24.04:20250105.1.0 |
There was a problem hiding this comment.
The container image tag ubuntu-24.04:20250105.1.0 seems to reference a future date. Please ensure that the image tag exists and is available for use. Additionally, consider using a stable version of the container image to avoid potential breaks due to future changes.
.github/workflows/ci.yml
Outdated
| @@ -5,7 +5,7 @@ on: | |||
|
|
|||
| jobs: | |||
| unit-tests: | |||
| runs-on: ubuntu-latest | |||
| runs-on: ubuntu-[email protected] | |||
There was a problem hiding this comment.
The runs-on key does not support specifying an image with a tag using @. You should use the container key to specify the image and tag.
.github/workflows/ci.yml
Outdated
| @@ -5,7 +5,7 @@ on: | |||
|
|
|||
| jobs: | |||
| unit-tests: | |||
| runs-on: ubuntu-latest | |||
| runs-on: ubuntu-24.04:20250105.1.0 | |||
There was a problem hiding this comment.
The runs-on value should use the correct syntax for specifying the runner image. It should be [email protected] instead of ubuntu-24.04:20250105.1.0. The colon (:) should be replaced with an at symbol (@).
| @@ -1,6 +1,6 @@ | |||
| module github.com/checkmarx/ast-cli | |||
|
|
|||
| go 1.23.3 | |||
| go 1.23.5 | |||
There was a problem hiding this comment.
The go version is updated from 1.23.3 to 1.23.5. Please ensure that this minor version bump does not introduce any compatibility issues with the project dependencies and that all tests pass with the new version.
.github/workflows/ci.yml
Outdated
| @@ -14,7 +14,17 @@ jobs: | |||
| with: | |||
| go-version-file: go.mod | |||
| - run: go version | |||
|
|
|||
There was a problem hiding this comment.
It seems like the original docker version command was removed without replacement. If the intention is to verify the Docker installation, consider adding a step to check the Docker version or ensure Docker is properly set up.
.github/workflows/ci.yml
Outdated
| uses: docker/setup-buildx-action@v1 | ||
|
|
||
| - name: Login to Docker Hub | ||
| uses: docker/login-action@dd4fa0671be5250ee6f50aedf4cb05514abda2c7 #v1 |
There was a problem hiding this comment.
Using a commit SHA (dd4fa0671be5250ee6f50aedf4cb05514abda2c7) directly for the docker/login-action is not recommended for maintainability. Prefer using a version tag (like v1) unless there is a specific reason for pinning to a commit.
| uses: docker/login-action@dd4fa0671be5250ee6f50aedf4cb05514abda2c7 #v1 | ||
| with: | ||
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_PASSWORD }} | ||
| - name: go test with coverage |
There was a problem hiding this comment.
The shell: bash directive is unnecessary here as bash is the default shell for GitHub Actions runners on Linux and macOS. You can remove this line unless there's a specific need for it.
| - name: go test with coverage | ||
| shell: bash | ||
| run: | | ||
| sudo chmod +x ./internal/commands/.scripts/up.sh |
There was a problem hiding this comment.
Using sudo within the GitHub Actions runner should generally be avoided unless necessary. Check if the chmod command can be run without sudo.
.github/workflows/ci.yml
Outdated
| @@ -14,7 +14,24 @@ jobs: | |||
| with: | |||
| go-version-file: go.mod | |||
| - run: go version | |||
|
|
|||
| - name: Setup Docker on macOS | |||
| uses: docker-practice/actions-setup-docker@master | |||
There was a problem hiding this comment.
It's not recommended to use the master branch of an action as it can introduce breaking changes without notice. Use a specific version or commit hash for docker-practice/actions-setup-docker to ensure stability.
.github/workflows/ci.yml
Outdated
|
|
||
| - name: Setup Docker on macOS | ||
| uses: docker-practice/actions-setup-docker@master | ||
| timeout-minutes: 12 |
There was a problem hiding this comment.
The timeout-minutes attribute should be aligned with the uses attribute for consistency and readability.
.github/workflows/ci.yml
Outdated
| uses: docker-practice/actions-setup-docker@master | ||
| timeout-minutes: 12 | ||
| - run: | | ||
| set -x |
There was a problem hiding this comment.
The set -x command will print all executed commands to the terminal, which can clutter the build logs. Consider removing it unless it's necessary for debugging purposes.
.github/workflows/ci.yml
Outdated
|
|
||
| docker version | ||
|
|
||
| docker run --rm hello-world |
There was a problem hiding this comment.
Running hello-world Docker image doesn't seem to serve a purpose in the CI workflow. If it's meant for testing the Docker setup, please ensure it's relevant to the project's CI goals.
| @@ -158,7 +175,7 @@ jobs: | |||
| username: ${{ secrets.DOCKER_USERNAME }} | |||
| password: ${{ secrets.DOCKER_PASSWORD }} | |||
| - name: Build the project | |||
| run: go build -o ./cx ./cmd | |||
| run: go build -o ./cx ./cmd | |||
There was a problem hiding this comment.
Consider using the -v flag with go build for verbose output to provide more context on the build process in the CI logs.
| @@ -175,7 +192,7 @@ jobs: | |||
| env: | |||
| TRIVY_SKIP_DB_UPDATE: true | |||
| TRIVY_SKIP_JAVA_DB_UPDATE: true | |||
|
|
|||
There was a problem hiding this comment.
It seems like there is an unnecessary change from a tab to a space on this line. Please revert it if it does not serve a specific purpose.
.github/workflows/ci.yml
Outdated
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_PASSWORD }} | ||
|
|
||
| - name: Login to Docker Hub |
There was a problem hiding this comment.
The Login to Docker Hub step appears to be redundant since you've already added a Log in to Docker Hub step above. Consider removing this duplicate step to streamline the workflow.
.github/workflows/ci.yml
Outdated
| run: | | ||
| sudo chmod +x ./internal/commands/.scripts/up.sh | ||
| ./internal/commands/.scripts/up.sh | ||
| sudo ./internal/commands/.scripts/up.sh |
There was a problem hiding this comment.
Using sudo to run up.sh might introduce security risks or may not be necessary. Ensure that the script has the appropriate permissions and only use sudo if absolutely required for the script to run correctly.
internal/commands/.scripts/up.sh
Outdated
| sudo go test $(go list ./... | grep -v "mock" | grep -v "wrappers" | grep -v "bitbucketserver" | grep -v "logger") -timeout 20m -coverprofile cover.out |
There was a problem hiding this comment.
Using sudo for running go test is generally not recommended as it can lead to permission issues and is not necessary for most testing scenarios. Please ensure that sudo is actually required for this operation and consider removing it if it's not necessary.
By submitting a PR to this repository, you agree to the terms within the Checkmarx Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
References
Testing
Checklist