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

Windows releases #6

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Windows releases #6

merged 8 commits into from
Sep 23, 2024

Conversation

gitworkflows
Copy link

@gitworkflows gitworkflows commented Sep 23, 2024

User description

Notes for Reviewers

This PR fixes #

Signed commits

  • [*] Yes, I signed my commits.

PR Type

enhancement, configuration changes


Description

  • Added support for building and testing Windows artifacts in the CI pipeline.
  • Updated golicenses.sh to include Windows platform support.
  • Enhanced the Makefile with a higher coverage threshold and fixed a typo.
  • Configured GoReleaser to handle Windows builds and archives.

Changes walkthrough 📝

Relevant files
Enhancement
golicenses.sh
Add Windows platform support in golicenses script               

golicenses.sh

  • Added support for Windows platform in the get_binaries function.
+1/-0     
Makefile
Improve Makefile with Windows support and typo fixes         

Makefile

  • Increased coverage threshold from 34 to 55.
  • Fixed typo in RELEASE_CMD.
  • Added Windows-specific test target.
  • Removed redundant exit commands.
  • +44/-47 
    Configuration changes
    pipeline.yaml
    Add Windows testing and setup in CI pipeline                         

    .github/workflows/pipeline.yaml

  • Added Go setup step for Windows.
  • Introduced a new job for testing Windows artifacts.
  • Updated release dependencies to include Windows artifacts.
  • +22/-1   
    goreleaser.yaml
    Configure GoReleaser for Windows builds                                   

    goreleaser.yaml

  • Added Windows build configuration.
  • Separated build configurations for Linux, Darwin, and Windows.
  • Introduced Windows-specific archive format.
  • +35/-6   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Summary by Sourcery

    Add Windows support for building and testing, enhance CI workflows to include Windows, and improve code quality by increasing the test coverage threshold. Refactor the Makefile for better readability and efficiency.

    New Features:

    • Add support for Windows builds and testing in the CI pipeline, including a new Windows-specific build configuration in goreleaser.yaml and a test-windows-artifacts job in the GitHub Actions workflow.

    Enhancements:

    • Increase the unit test coverage threshold from 34% to 55% in the Makefile to ensure higher code quality.
    • Refactor the Makefile to remove redundant exit commands and improve readability by simplifying directory creation and command execution.

    CI:

    • Introduce a new CI job for testing Windows artifacts, ensuring compatibility and functionality on Windows platforms.
    • Update the GitHub Actions workflow to include Go setup steps for Windows, macOS, and Linux environments, standardizing the Go version used across different platforms.

    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Copy link

    sourcery-ai bot commented Sep 23, 2024

    Reviewer's Guide by Sourcery

    This pull request adds support for Windows releases and makes several improvements to the build process and CI pipeline. The changes include updating the Makefile, goreleaser configuration, GitHub Actions workflow, and the golicenses shell script.

    File-Level Changes

    Change Details Files
    Added support for Windows builds and releases
    • Added a new build configuration for Windows in goreleaser.yaml
    • Updated the golicenses.sh script to include Windows/amd64 platform
    • Added a new CI job for testing Windows artifacts
    goreleaser.yaml
    .github/workflows/pipeline.yaml
    golicenses.sh
    Updated build configurations and artifact naming
    • Renamed build configurations to linux-build, darwin-build, and windows-build
    • Updated archive configurations to use specific builds
    • Changed artifact naming convention in CI test jobs
    goreleaser.yaml
    .github/workflows/pipeline.yaml
    Makefile
    Improved Makefile structure and error handling
    • Removed explicit error checking and exit calls
    • Simplified some command structures
    • Updated PHONY targets
    • Changed RELEASE_CMD to RELAESE_CMD (likely a typo)
    Makefile
    Enhanced CI pipeline
    • Added Go setup steps to test jobs
    • Included the new Windows test job in the release job dependencies
    .github/workflows/pipeline.yaml
    Increased code coverage threshold
    • Changed COVERAGE_THRESHOLD from 34 to 55
    Makefile

    Tips
    • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
    • Continue your discussion with Sourcery by replying directly to review comments.
    • You can change your review settings at any time by accessing your dashboard:
      • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
      • Change the review language;
    • You can always contact us if you have any questions or feedback.

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Oversight
    The new Windows test job is not included in the 'needs' array for the release job, potentially allowing releases without Windows artifact testing.

    Code Duplication
    The 'bootstrap' target is defined twice, which may lead to confusion or unexpected behavior.

    Copy link

    codiumai-pr-agent-free bot commented Sep 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    ✅ Fix typo in variable name
    Suggestion Impact:The typo in the variable name RELAESE_CMD was corrected to RELEASE_CMD, as suggested.

    code diff:

    -RELAESE_CMD=$(BIN)/goreleaser --rm-dist
    +RELEASE_CMD=$(BIN)/goreleaser --rm-dist

    The variable name RELAESE_CMD contains a typo. It should be RELEASE_CMD. This typo
    could lead to unexpected behavior in the Makefile.

    Makefile [21]

    -RELAESE_CMD=$(BIN)/goreleaser --rm-dist
    +RELEASE_CMD=$(BIN)/goreleaser --rm-dist
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Fixing the typo is crucial as it directly affects the execution of the Makefile, preventing potential errors during the release process.

    10
    Enhancement
    Simplify platform-specific binary name assignment

    Consider adding a check for the Windows platform before setting the BINARIES
    variable. This will ensure that the script only sets the .exe extension for Windows
    and maintains the correct binary name for other platforms.

    golicenses.sh [64-70]

     case "$PLATFORM" in
    -  darwin/amd64) BINARIES="golicenses" ;;
    -  linux/amd64) BINARIES="golicenses" ;;
    -  darwin/arm64) BINARIES="golicenses" ;;
    -  linux/arm64) BINARIES="golicenses" ;;
    +  darwin/amd64|linux/amd64|darwin/arm64|linux/arm64) BINARIES="golicenses" ;;
       windows/amd64) BINARIES="golicenses.exe" ;;
       *)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion simplifies the case statement by grouping similar cases, which improves readability and maintainability without changing functionality.

    8
    Maintainability
    Use YAML anchors and aliases to reduce configuration duplication

    Consider using YAML anchors and aliases to define common configurations across
    different builds, reducing duplication and improving maintainability.

    goreleaser.yaml [7-32]

     builds:
    -  - id: linux-build
    +  - &build-template
         binary: golicenses
    -    goos:
    -      - linux
    -    goarch:
    +    ldflags: &build-ldflags -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.buildTime={{.Date}}`.
    +    goarch: &common-arch
           - amd64
           - arm64
    -    ldflags: &build-ldflags -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.buildTime={{.Date}}`.
     
    -  - id: darwin-build
    -    binary: golicenses
    -    goos:
    -      - darwin
    -    goarch:
    -      - amd64
    -      - arm64
    -    ldflags: *build-ldflags
    +  - <<: *build-template
    +    id: linux-build
    +    goos: [linux]
     
    -  - id: windows-build
    -    binary: golicenses
    -    goos:
    -      - windows
    -    goarch:
    -      - amd64
    -    ldflags: *build-ldflags
    +  - <<: *build-template
    +    id: darwin-build
    +    goos: [darwin]
     
    +  - <<: *build-template
    +    id: windows-build
    +    goos: [windows]
    +    goarch: [amd64]
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The use of YAML anchors and aliases can significantly reduce duplication and improve maintainability, but the existing code is already functional and clear. The suggestion is a good practice but not critical.

    7
    ✅ Use a matrix strategy for Go setup steps across jobs
    Suggestion Impact:The suggestion to use a matrix strategy for Go setup steps was implemented by updating the Go setup to use a matrix for different platforms and Go versions, reducing duplication across jobs.

    code diff:

         strategy:
    @@ -10,16 +12,17 @@
             platform: [ubuntu-latest, macos-latest]
         runs-on: ${{ matrix.platform }}
         steps:
    +      - name: Checkout code
    +        uses: actions/checkout@v2
     
    -      - uses: actions/setup-go@v1
    +      - name: Set up Go
    +        uses: actions/setup-go@v2
             with:
               go-version: ${{ matrix.go-version }}

    Consider using a matrix strategy for the Go setup steps across different jobs to
    reduce code duplication and improve maintainability.

    .github/workflows/pipeline.yaml [78-80]

    -- uses: actions/setup-go@v1
    +- uses: actions/setup-go@v3
       with:
         go-version: '1.18.x'
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While using a matrix strategy can reduce duplication, the suggestion does not provide a concrete implementation, and the existing code is functional.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link

    @sourcery-ai sourcery-ai bot left a comment

    Choose a reason for hiding this comment

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

    Hey @gitworkflows - I've reviewed your changes - here's some feedback:

    Overall Comments:

    • There's a typo in the Makefile: RELAESE_CMD should be RELEASE_CMD. This could cause issues with the release process.
    • Some error checking has been removed from the Makefile commands (e.g., || exit 1 removed from mkdir commands). Consider keeping these to ensure the build fails appropriately on errors.
    • The code coverage threshold has been significantly increased from 34% to 55%. Please confirm if this is intentional and achievable.
    Here's what I looked at during the review
    • 🟡 General issues: 1 issue found
    • 🟢 Security: all looks good
    • 🟢 Testing: all looks good
    • 🟢 Complexity: all looks good
    • 🟢 Documentation: all looks good

    Sourcery is free for open source - if you like our reviews please consider sharing them ✨
    Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

    Makefile Outdated

    RELEASE_CMD=$(BIN)/goreleaser --rm-dist
    RELAESE_CMD=$(BIN)/goreleaser --rm-dist
    Copy link

    Choose a reason for hiding this comment

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

    issue (typo): Fix typo in RELAESE_CMD variable name

    This typo could cause issues when trying to release the project. Please correct it to RELEASE_CMD.

    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    Signed-off-by: gitworkflows <118260833+gitworkflows@users.noreply.github.com>
    @gitworkflows gitworkflows merged commit f3d1abd into master Sep 23, 2024
    15 checks passed
    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.

    1 participant