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

Logentries line-only logopt fix to maintain backwards compatibility #35628

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
5 participants
@ikarpovich
Contributor

ikarpovich commented Nov 28, 2017

- 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 run without specifying line-only logopt at all.

- A picture of a cute animal (not mandatory but encouraged)

image

Signed-off-by: Igor Karpovich igor@karpovich.me

@thaJeztah

left one comment, but LGTM after that's addressed; can you also squash your commits? I think a single commit should be ok for this change (including the added test)

if info.Config[lineonly] != "" {
if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); err != nil {
return nil, errors.Wrap(err, "error parsing lineonly option")
}

This comment has been minimized.

@thaJeztah

thaJeztah Nov 29, 2017

Member

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)
}
@thaJeztah

thaJeztah Nov 29, 2017

Member

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)
}

This comment has been minimized.

@boaz1337

boaz1337 Nov 30, 2017

Contributor

Can I also suggest changing the two ifs into one:

if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); info.Config[lineonly] != "" && err != nil {
    ...
}
@boaz1337

boaz1337 Nov 30, 2017

Contributor

Can I also suggest changing the two ifs into one:

if lineOnly, err = strconv.ParseBool(info.Config[lineonly]); info.Config[lineonly] != "" && err != nil {
    ...
}

This comment has been minimized.

@ikarpovich

ikarpovich Nov 30, 2017

Contributor

@ripcurld0 Sorry but don't agree as the (unnecessary) cast statement would be executed in any case this way.

@ikarpovich

ikarpovich Nov 30, 2017

Contributor

@ripcurld0 Sorry but don't agree as the (unnecessary) cast statement would be executed in any case this way.

This comment has been minimized.

@ikarpovich

ikarpovich Nov 30, 2017

Contributor

@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 :)

@ikarpovich

ikarpovich Nov 30, 2017

Contributor

@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 :)

@boaz1337

LGTM minus @thaJeztah comments

Logentries line-only logopt fix to maintain backwards compatibility
Changed logic to ignore empty value

Fixes #35626

Signed-off-by: Igor Karpovich <igor@karpovich.me>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 30, 2017

Member

@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 :)

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

Member

thaJeztah commented Nov 30, 2017

@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 :)

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

@thaJeztah

LGTM

@thaJeztah thaJeztah removed this from backlog in maintainers-session Nov 30, 2017

@vieux

vieux approved these changes Nov 30, 2017

LGTM

@vieux vieux merged commit 3a9ab94 into moby:master Nov 30, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38107 has succeeded
Details
janky Jenkins build Docker-PRs 46821 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7230 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18376 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7049 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment