feat!: add Go modules and Go 1.19 compatibility #114
Conversation
zwartho
changed the title
zwartho
changed the title
zwartho
changed the title
|
@marcoshuck this is ready for review. |
| @@ -36,14 +36,39 @@ jobs: | |||
| - name: Run Tests | |||
| run: make test | |||
|
|
|||
| test-v3: | |||
There was a problem hiding this comment.
Why are we running this in a separate job? Consider including the respective Go versions in the original test job. Keep in mind while performing these changes that Go 1.20 is out already.
There was a problem hiding this comment.
Ditto to the reason I explained in go commands comment.
|
|
||
| test: install | ||
| go test -race -cover -v ./... |
There was a problem hiding this comment.
I understand now, this change was intentional due to the subfolder, if we remove the subfolder altogether, we can revert this change.
| @@ -0,0 +1,5 @@ | |||
| module github.com/sendgrid/rest/v3 | |||
There was a problem hiding this comment.
I don't think it's a good idea to have a different folders. We can simply replace the version in the original go.mod file instead. Please revert these changes in the v3 folder.
There was a problem hiding this comment.
Even though the https://go.dev/blog/v2-go-modules recommends having different folders, that's for projects that are actually introducing breaking changes, and this is not the case in this PR.
There was a problem hiding this comment.
@marcoshuck Thanks for your review. If you take a look at these failed jobs, breaking changes are seemingly apparent when referencing the io module. This SO question explains it nicely, but essentially the only Go changes made in this MR were replacing references to the deprecated ioutil module. I could not get tests for certain versions of Go passing without utilizing the v3 directory and splitting up the test job into two separate jobs. So in order to prevent breaking functionality for users of Go 1.14, 1.15, etc., I believe the need for the v3 directory and separate test jobs is necessary. If you have any suggestions to mitigate this issue in a different way, please let me know and I can accommodate.
There was a problem hiding this comment.
That's a very a good point. We need to have this discussion with some of the members from the @sendgrid team, my 2c: We should replace ioutil with io package references or any other alternative that makes it work. Bumping to a new major version should allow us to deprecate previous Go versions.
There was a problem hiding this comment.
@twilio-dx @SendGridDX Any suggested next steps?
Fixes
This PR is based off the changes introduced in #112 by @AlaricWhitney.
Adds Go Modules, and also adds a v3 directory as per current Golang standards as per https://go.dev/blog/v2-go-modules.
This PR also makes the following changes:
Checklist
If you have questions, please file a support ticket.