Skip to content
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

Add debugging commands into taskfiles, adjust configs #6307

Closed
wants to merge 2 commits into from

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented May 23, 2024

PR Type

enhancement, tests, documentation


Description

  • Updated Go version from 1.21.4 to 1.22.3 in Dockerfile.
  • Added git installation and git config command in Dockerfile.
  • Removed coprocess and grpc tags from Makefile.
  • Added new tasks test:build and test:build-binary for Go 1.22 in Taskfile.
  • Updated build flags in Goreleaser configuration.
  • Added instructions for running tests and debugging with delve in tracing tests README.
  • Added install:delve task and debug support in tracing Taskfile.
  • Added new Docker Compose configuration for debugging with delve.

Changes walkthrough 📝

Relevant files
Enhancement
Dockerfile
Update Go version and add git configuration in Dockerfile

Dockerfile

  • Updated Go version from 1.21.4 to 1.22.3.
  • Added git to the list of installed packages.
  • Added git config command to set the safe directory.
  • +5/-3     
    Makefile
    Adjust build tags in Makefile                                                       

    Makefile

    • Removed coprocess and grpc from the TAGS variable.
    +1/-1     
    Taskfile.yml
    Add build tasks for Go 1.22 in Taskfile                                   

    Taskfile.yml

  • Added new tasks test:build and test:build-binary for building with Go
    1.22.
  • +32/-0   
    goreleaser.yml
    Update build flags in Goreleaser configuration                     

    ci/goreleaser/goreleaser.yml

  • Added -race, -buildvcs=false, and -gcflags -l -N flags to the build
    configuration.
  • +4/-0     
    Taskfile.yml
    Add delve installation task and debug support in Taskfile

    ci/tests/tracing/Taskfile.yml

  • Added debug and output variables.
  • Added install:delve task to produce debug image with delve.
  • +22/-5   
    docker-compose.debug.yml
    Add Docker Compose configuration for debugging                     

    ci/tests/tracing/docker-compose.debug.yml

    • Added new Docker Compose file for debugging with delve.
    +137/-0 
    Documentation
    README.md
    Add debugging instructions to tracing tests README             

    ci/tests/tracing/README.md

    • Added instructions for running tests and debugging with delve.
    +77/-3   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @titpetric titpetric requested review from a team as code owners May 23, 2024 13:59
    Copy link
    Contributor

    github-actions bot commented May 23, 2024

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Description updated to latest commit (bfb740a)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and configurations across different technologies (Docker, Makefile, Taskfile, GoReleaser, and documentation). Understanding the implications of these changes requires a good grasp of the project's build and deployment processes, as well as debugging configurations.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The git config --global --add safe.directory {{.buildDir}} command in the test:build and test:build-binary tasks of the Taskfile might not behave as expected when run in a Docker container if the {{.buildDir}} placeholder is not properly replaced at runtime.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileDockerfile
    suggestion      

    Consider pinning the git package to a specific version to ensure consistent builds and avoid potential issues with unexpected updates. [important]

    relevant lineRUN apt update && apt install git build-essential zlib1g-dev libncurses5-dev libgdbm-dev libnss3-dev libssl-dev libsqlite3-dev libreadline-dev libffi-dev curl wget libbz2-dev -y

    relevant fileTaskfile.yml
    suggestion      

    Ensure that the {{.buildDir}} variable is properly expanded in the Docker run command to avoid runtime errors or unexpected behavior. Consider adding a validation step or a default value. [important]

    relevant line-v $PWD:{{.buildDir}} \

    relevant fileci/goreleaser/goreleaser.yml
    suggestion      

    The addition of -gcflags and -l -N without values might be an oversight. If these are intended to be used, ensure they are correctly configured to avoid build errors. [important]

    relevant line- -gcflags

    relevant fileci/tests/tracing/README.md
    suggestion      

    Clarify the usage of GATEWAY_IMAGE environment variable in the debugging section to ensure users understand how to use their own Docker image for testing. [medium]

    relevant line- You are able to override the docker image by setting `GATEWAY_IMAGE= task`.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Restore removed build tags to avoid unintended functionality loss

    Restore the removed tags coprocess and grpc in the TAGS variable unless specifically
    intended to remove them, as this change might impact the functionality that depends on
    these tags.

    Makefile [12]

    -TAGS=goplugin
    +TAGS=coprocess grpc goplugin
     
    Suggestion importance[1-10]: 10

    Why: Restoring the removed build tags is crucial to avoid unintended functionality loss, ensuring that all necessary features are included in the build.

    10
    Performance
    Combine apt update and apt install into a single RUN statement and clean up afterward to reduce image layers and size

    Combine the apt update and apt install commands into a single RUN statement to reduce the
    number of layers in the Docker image, which can help in optimizing the build process and
    the size of the final image.

    Dockerfile [34]

    -RUN apt update && apt install git build-essential zlib1g-dev libncurses5-dev libgdbm-dev libnss3-dev libssl-dev libsqlite3-dev libreadline-dev libffi-dev curl wget libbz2-dev -y
    +RUN apt update && apt install -y git build-essential zlib1g-dev libncurses5-dev libgdbm-dev libnss3-dev libssl-dev libsqlite3-dev libreadline-dev libffi-dev curl wget libbz2-dev && rm -rf /var/lib/apt/lists/*
     
    Suggestion importance[1-10]: 9

    Why: Combining apt update and apt install into a single RUN statement and cleaning up afterward is a good practice to reduce the number of layers and the size of the Docker image, optimizing the build process.

    9
    Best practice
    Use a more specific tag for the Debian base image to ensure consistent builds

    Consider using a more specific tag for the Debian base image to ensure consistent and
    predictable builds. Using a generic tag like debian:bookworm can lead to unexpected
    changes if the tag is updated upstream.

    Dockerfile [17]

    -FROM debian:bookworm
    +FROM debian:bookworm-20230501
     
    Suggestion importance[1-10]: 8

    Why: Using a more specific tag for the Debian base image is a best practice that ensures consistent and predictable builds, reducing the risk of unexpected changes from upstream updates.

    8
    Maintainability
    Use variables for Go version and Debian version in Docker tags to enhance flexibility and maintainability

    Avoid hardcoding the Go version and the Docker image tag within the task commands.
    Instead, use variables to make the build process more flexible and maintainable.

    Taskfile.yml [58]

    -tykio/golang-cross:1.22-bookworm
    +tykio/golang-cross:{{.GO_VERSION}}-{{.DEBIAN_VERSION}}
     
    Suggestion importance[1-10]: 7

    Why: Using variables for the Go version and Debian version in Docker tags enhances flexibility and maintainability, making it easier to update and manage the build process.

    7

    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    Copy link

    sonarcloud bot commented May 23, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    2 Security Hotspots

    See analysis details on SonarCloud

    @titpetric titpetric closed this Jul 8, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants