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.

Sign up for GitHub

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

Jump to bottom

update go versions to go1.19, go1.20, go1.21, and add go.mod #114

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

update go versions to go1.19, go1.20, go1.21, and add go.mod #114

wants to merge 11 commits into from

Conversation

thaJeztah
Copy link

This was referenced Dec 10, 2022
Closed
Merged
@fujimotos fujimotos self-requested a review December 12, 2022 11:02
@thaJeztah thaJeztah mentioned this pull request Jul 1, 2023
Merged
@thaJeztah thaJeztah force-pushed the update_go_versions branch from fcdbb5a to 9c5d0b9 Compare July 1, 2023 14:12
@thaJeztah thaJeztah changed the title update go versions to go1.18, go1.19, and add go.mod update go versions to go1.19, go1.20, and add go.mod Jul 1, 2023
@thaJeztah thaJeztah force-pushed the update_go_versions branch from 9c5d0b9 to 3af10e4 Compare July 1, 2023 14:16
@thaJeztah
Copy link
Author

Builds are happy now; https://github.com/thaJeztah/fluent-logger-golang/actions/runs/5431484742

Screenshot 2023-07-01 at 16 23 20

@thaJeztah
Copy link
Author

Moving this out of draft in favour of the earlier PR @fujimotos PTAL 🤗

@thaJeztah thaJeztah marked this pull request as ready for review July 1, 2023 14:24
@tagomoris
Copy link
Member

Now, I'm not an active maintainer of this repository.
@fujimotos ?

@AkihiroSuda
Copy link

AkihiroSuda commented Aug 16, 2023 •
edited
Loading

@tagomoris @fujimotos
Any chance to get somebody to merge this PR? (and tag a new release)

thaJeztah added 11 commits August 16, 2023 17:32
@thaJeztah
gha: update actions/checkout@v3, actions/setup-go@v4
050b7b9 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
all: gofmt code for current go versions
edf7dad 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
fix example in README
940c2a6 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
use string-literals to prevent having to escape quotes
b8e5e2f 
This makes the code and examples slightly more readable.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
tests: make sure to capture test-case in loop
4ff6d97 
TestNoPanicOnAsyncClose was not capturing the testcase, but running
with t.Parallel().

Also updated other tests to use the same approach, and renamed variables
for consistency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
remove use of deprecated github.com/bmizerany/assert package
08687f1 
The github.com/bmizerany/assert module has been deprecated and is no
longer maintained. In addition, the dependency brings various indirect
dependencies with it;

    module github.com/fluent/fluent-logger-golang

    go 1.19

    require (
        github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869
        github.com/tinylib/msgp v1.1.6
    )

    require (
        github.com/kr/pretty v0.3.1 // indirect
        github.com/kr/text v0.2.0 // indirect
        github.com/philhofer/fwd v1.1.1 // indirect
        github.com/rogpeppe/go-internal v1.9.0 // indirect
    )

The assertion package itself also looks to have some issues. For example,
it's not marked as a `t.Helper()`, and while it includes the location of
the error in the output, the failures are prefixed with the location of
the assertion itself, which is somewhat confusing:

    === RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:275
        assert.go:62: !  Unexpected: <24224>
    --- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

    === RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:306
        assert.go:62: !  Unexpected: <"unix">
    --- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromArguments
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:327
        assert.go:62: !  Unexpected: <6666>
    --- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:333
        assert.go:62: !  Unexpected: <true>
    --- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:653
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:655
    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        assert.go:62: !  Unexpected: <&errors.errorString{s:"fluent#appendBuffer: Logger already closed"}>
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        assert.go:62: !  Unexpected: <<nil>>

While a good assertion library can help (for example by printing a rich
diff output if a struct does not match), a look at how it's used shows
that most cases are comparing primitive types (int, string, bool). This
patch swaps the library for a local `assertEqual()` utility. With this
patch, test-failures look like the example below:

    === RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
        fluent_test.go:281: got: '24224', expected: '24224'
        fluent_test.go:282: got: '127.0.0.1', expected: '127.0.0.1'
        fluent_test.go:283: got: '3s', expected: '3s'
        fluent_test.go:284: got: '0s', expected: '0s'
        fluent_test.go:285: got: '8192', expected: '8192'
        fluent_test.go:286: got: 'tcp', expected: 'tcp'
        fluent_test.go:287: got: '', expected: ''
    --- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

    === RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
        fluent_test.go:312: got: 'unix', expected: 'unix'
        fluent_test.go:313: got: '/tmp/fluent-logger-golang.sock', expected: '/tmp/fluent-logger-golang.sock'
    --- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromArguments
        fluent_test.go:333: got: '6666', expected: '6666'
        fluent_test.go:334: got: 'foobarhost', expected: 'foobarhost'
    --- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
        fluent_test.go:339: got: 'true', expected: 'true'
    --- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

    === RUN   TestJsonConfig
        fluent_test.go:441: got: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}', expected: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}'
    --- FAIL: TestJsonConfig (0.00s)

    === CONT  TestPostWithTime/without_Async
        fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'
    === CONT  TestPostWithTime/with_Async
        fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'

    === CONT  TestReconnectAndResendAfterTransientFailure/with_Async
        fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
    === CONT  TestReconnectAndResendAfterTransientFailure/without_Async
        fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'
    === CONT  TestReconnectAndResendAfterTransientFailure/with_Async
        fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'

    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        fluent_test.go:657: got: 'fluent#appendBuffer: Logger already closed', expected: 'fluent#appendBuffer: Logger already closed'
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        fluent_test.go:659: got: '<nil>', expected: '<nil>'

The list of dependencies have also been reduced with this patch:

    module github.com/fluent/fluent-logger-golang

    go 1.19

    require github.com/tinylib/msgp v1.1.6

    require github.com/philhofer/fwd v1.1.1 // indirect

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
gha: update go versions to 1.19, 1.20, 1.21
fe43282 
Note that go1.13 has reached EOL a long time ago; removing that version,
as packages are no longer compatible with it:

    go get github.com/philhofer/fwd
    # golang.org/x/tools/internal/gocommand
    Error: ../../../go/src/golang.org/x/tools/internal/gocommand/invoke.go:399:62: undefined: os.ErrProcessDone
    Error: Process completed with exit code 2.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
add go.mod
714b11b 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
gha: remove step to manually install dependencies
ce664d8 
the go module should take care of downloading the module.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
go generate
152f598 
update generated code with the latest version of msgp:

   go generate -x ./...

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
remove unused defaultSubSecondPrecision const
3f4e90b 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah changed the title update go versions to go1.19, go1.20, and add go.mod update go versions to go1.19, go1.20, go1.21, and add go.mod Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@fujimotos fujimotos Awaiting requested review from fujimotos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thaJeztah @tagomoris @AkihiroSuda