Conversation
mathieu-lemay
left a comment
There was a problem hiding this comment.
For what it's worth, this looks perfectly fine to me. My only "concern" would be the bump of the Go version itself, I personally think it's fine.
| module github.com/wiremock/go-wiremock | ||
|
|
||
| go 1.20 | ||
| go 1.24.0 |
There was a problem hiding this comment.
This is the only thing I can see as problematic with this change, it now requires go 1.24. I personally don't mind, but I'm not sure what version of go do the owners of the project want to target.
There was a problem hiding this comment.
That is understandable, my initial worry was about testcontainers dependency that is very outdated and has CVEs. I ended up bumping more things, with a worry that this would again not be bumped for some time. But for me is also fine if we keep Go version as is and just try to bump testcontainers.
|
Hi, I have approved the workflow runs and it looks like we have some linting errors. Are those something you want to address before we merge? |
Hello, thanks for allowing the workflow to run. I can try to fix the lint, but what would be your input/suggestion here? a) Basically ignore close errors via b) Capture close errors in return value Requires changing all function signatures to use named return values. Returns close errors only when the main operation succeeded. c) Any other option, e.g. just logging the error Would require adding a logging dependency (I don't see any logger currently used in the project). |
|
If I may chime in, those "errors" were already present, the only change is that they are now reported by the linter, I assume because golangci-lint was updated. I consider them very harmless in the sense that there's nothing really actionnable on a body close error, except logging it and carrying on. In my team at work, we usually silence them: defer res.Body.Close() //nolint:errcheck // there is nothing to do even if an error occurs on close functionIf we don't want to do that, then I'd go with option |
Yup, exactly those "errors" were indeed already there and with the update I did on the linter, it now complains about them. Since I saw them already there was in doubt what the preferred approach was. |
| req, err := http.NewRequest(http.MethodDelete, fmt.Sprintf("%s/%s", c.url, wiremockAdminMappingsURN), nil) | ||
| if err != nil { | ||
| return fmt.Errorf("build cleare Request error: %w", err) | ||
| return fmt.Errorf("build clear Request error: %w", err) |
There was a problem hiding this comment.
I was just looking at the code and noticed this. It makes me happy to see it fixed 😄
|
If everyone is happy with this I can merge it in |
|
Thank you all for helping get this merged |
|
Thanks for reviewing and merging. Can this be released? |
|
I'll release a new version today |
References
See #39
Submitter checklist
#help-contributingor a project-specific channel like#wiremock-java