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

chore: final cleanups of the go.mod file #9663

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

plumpy
Copy link
Collaborator

@plumpy plumpy commented Jan 14, 2025

  • Two deps moved, pin the old paths at the last version from that path
    • this was suggested by the mergo readme
    • update the Skaffold import to use the new path

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi @plumpy, @reviewers,

I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary of the changes for you and other reviewers to get up to speed:

This pull request, "chore: final cleanups of the go.mod file", aims to perform some final cleanups on the go.mod file. The changes include:

  • Removing an outdated exclude: The line exclude github.com/karrick/godirwalk v1.17.0 has been removed from go.mod.
  • Moving and pinning dependencies: Two dependencies have been moved to new paths. The old paths are pinned to their last known versions:
    • github.com/alessio/shellescape is replaced with github.com/alessio/shellescape v1.4.2
    • github.com/imdario/mergo is replaced with github.com/imdario/mergo v0.3.16
  • Updating other dependencies: Several other dependencies have been updated to their latest versions. This includes updates to github.com/aws/aws-sdk-go-v2/config, github.com/aws/aws-sdk-go-v2/credentials, github.com/aws/aws-sdk-go-v2/service/ecr, and github.com/aws/aws-sdk-go-v2/service/sts, among others. The specific version changes are detailed in the diffs for go.mod and go.sum.
  • Updating import path: The import path for the mergo library in pkg/skaffold/config/util.go has been updated from github.com/imdario/mergo to dario.cat/mergo (line 30).
  • Improvements to godirwalk documentation and error handling: The documentation for the godirwalk library has been improved to include instructions on preventing resource leaks (lines 26-29, 58-63, 81-83, 118-121 of scandir_unix.go and lines 21-24, 58-67, 72-74, 109-112 of scandir_windows.go). Error handling has also been improved in scandir_unix.go (lines 102-111, 150-156) and scandir_windows.go (lines 96-107).

The changes to go.sum reflect the updated dependency versions. The modules.txt file also shows the changes made to the dependencies.

I've reviewed the changes and they appear to be well-structured and thoroughly documented. I'll provide a more detailed review shortly.

And now, a little haiku to lighten the mood:

Go modules clean,
Dependencies aligned now,
Code flows smoothly.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This PR cleans up the go.mod file by removing an outdated exclude directive and pinning dependencies that have changed paths. This is good practice for dependency management and ensures reproducible builds. The changes are well-justified and follow the recommendations from the mergo readme.

@plumpy plumpy force-pushed the last_final_dep_fixes branch from a05aad3 to 65cbaab Compare January 14, 2025 22:10
@plumpy plumpy marked this pull request as ready for review January 14, 2025 22:44
@plumpy plumpy requested a review from a team as a code owner January 14, 2025 22:44
@plumpy plumpy requested review from ericzzzzzzz and alphanota and removed request for ericzzzzzzz January 14, 2025 22:44
@plumpy plumpy merged commit 4459228 into GoogleContainerTools:main Jan 15, 2025
13 checks passed
@plumpy plumpy deleted the last_final_dep_fixes branch January 15, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants