Logentries line-only logopt fix to maintain backwards compatibility #35628
Conversation
| @@ -51,7 +51,10 @@ func New(info logger.Info) (logger.Logger, error) { | |||
| } | |||
| var lineOnly bool | |||
| if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); err != nil { | |||
| return nil, errors.Wrap(err, "error parsing lineonly option") | |||
There was a problem hiding this comment.
Instead of this, would it work to just check if the info.Config[lineonly] is empty before trying to parse it?
| if info.Config[lineonly] != "" { | ||
| if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); err != nil { | ||
| return nil, errors.Wrap(err, "error parsing lineonly option") | ||
| } |
There was a problem hiding this comment.
Can you add a minimum test-case for this? Could be as simple as;
package logentries
import (
"testing"
"github.com/docker/docker/daemon/logger"
"github.com/stretchr/testify/require"
)
func TestNewWithEmptyOptions(t *testing.T) {
_, err := New(logger.Info{})
require.NoError(t, err)
}
There was a problem hiding this comment.
Can I also suggest changing the two ifs into one:
if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); info.Config[lineonly] != "" && err != nil {
...
}There was a problem hiding this comment.
@ripcurld0 Sorry but don't agree as the (unnecessary) cast statement would be executed in any case this way.
There was a problem hiding this comment.
@thaJeztah You know I even have done this and this obviously worked but then I noticed that actual TCP connection to Logentries is done in New() as well. Logentries driver used is a third party with service URL hardcoded so in this case the whole driver has to be mocked to test this properly. Unless we want to test actual Logentries connection on each test pass :)
Changed logic to ignore empty value Fixes #35626 Signed-off-by: Igor Karpovich <[email protected]>
Ah, you're right; given that this is a trivial change, I think it's ok for now to merge this without a test added |
- What I did
Fixes #35626
Logentries line-only logopt breaks backwards compatibility, logentries driver won't start without this option so existing swarm stacks will stop working after the engine update.
- How I did it
Log driver will now ignore empty value and drop an error only in case of invalid boolean value.
- How to verify it
Try to do
docker runwithout specifying line-only logopt at all.- A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Igor Karpovich [email protected]