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

ISSUE-887: Fix error logging on docker push task when auth config fil… #894

Closed
wants to merge 1 commit into from
Closed

ISSUE-887: Fix error logging on docker push task when auth config fil… #894

wants to merge 1 commit into from

Conversation

mikeykoelbl
Copy link

…e is not present

This fixes some of the problems mentioned in ISSUE-887, that logging

  • If the authentication configuration file cannot be found, just return the default configuration without logging (either loglevel warn or loglevel error)
  • If the authentication configuration file cannot be parsed, we will go ahead with the registry push (as before) put log as a warn. The loglevel "warn" is correct for the situation "we found a problem but tried to continue"
  • I have added tests that verify that logs are written to the correct log level.

@mikeykoelbl mikeykoelbl mentioned this pull request Dec 6, 2019
Closed
@mikeykoelbl
Copy link
Author

mikeykoelbl commented Dec 6, 2019 •
edited
Loading

I have noticed that you have changed the code I was previously working on EDIT: sorry, I meant to say, the code in the same area I was working on - I shall iron our the merge conflict and push again.

@cdancy cdancy added this to the v6.1.0 milestone Dec 6, 2019
@mikeykoelbl
Copy link
Author

okay, I don't know why the functional tests have failed. I have not yet found anything but will look into it later in a few hours' time.

@mikeykoelbl
Copy link
Author

mikeykoelbl commented Dec 6, 2019 •
edited
Loading

Okay, I cannot reproduce the same error in the functional tests that was shown in this build:

com.bmuschko.gradle.docker.tasks.container.DockerWaitHealthyContainerFunctionalTest > Invoke onNext periodically passing the health status FAILED

My tests run properly, but only when I start with a clean docker slate and with max-workers=1

lucinda:gradle-docker-plugin mrk-hre$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.14.6
BuildVersion:	18G95

Docker version 19.03.1
Java 1.8.0_232 (AdoptOpenJDK)
There is no $HOME/.docker/
I have copied src/functTest/resources/auth/config.json to $HOME/.docker/config.json

If you need any other information, please let me know.

@bmuschko
Copy link
Owner

bmuschko commented Dec 6, 2019

The failing functional tests is probably a flaky test case. You can ignore that for now. I will look into the root cause and fix for it soon. Moreover, I will review your code over the weekend.

bmuschko
bmuschko requested changes Dec 8, 2019
View reviewed changes
Copy link
Owner

@bmuschko bmuschko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should start by adding test cases for the reported issues from users. Only then we should get started and jump to conclusions/change the code. Switching the log level from error to warn will only mask real issues by basically ignoring them.

src/main/groovy/com/bmuschko/gradle/docker/internal/RegistryAuthLocator.groovy
@@ -127,7 +130,8 @@ class RegistryAuthLocator {
}

} catch(Exception ex) {
logger.error('Failure when attempting to lookup auth config ' +
logger.warn('Failure when attempting to lookup auth config ' +
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'd not want to change the log level for anything that might go wrong. It probably indicates a real issue. If the parsing of the JSON file doesn't work properly then that's a bug.

So I'd still want to see an error for the following cases:

  1. The JSON cannot be parsed.
  2. The auth config cannot be determined.
  3. The credentials cannot be resolved.

IMHO those are all valid use cases. So if you have a config file then I'd expect it to work.

src/main/groovy/com/bmuschko/gradle/docker/internal/RegistryAuthLocator.groovy
@@ -106,6 +106,9 @@ class RegistryAuthLocator {
configFile.exists() ? 'exists' : 'does not exist',
commandPathPrefix)

if (!(configFile.exists())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am OK with adding this in, however, it will only cover some of the issues users reported.

src/main/groovy/com/bmuschko/gradle/docker/internal/RegistryAuthLocator.groovy
@@ -106,6 +106,9 @@ class RegistryAuthLocator {
configFile.exists() ? 'exists' : 'does not exist',
commandPathPrefix)

if (!(configFile.exists())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use isFile() instead.

@bmuschko
Copy link
Owner

bmuschko commented Dec 9, 2019

Thanks for the pull request. I just committed the change separately: fe2c7bc

@bmuschko bmuschko closed this Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmuschko bmuschko bmuschko requested changes

Assignees

@bmuschko bmuschko

@cdancy cdancy

@orzeh orzeh

Labels
bug enhancement
Projects
None yet
Milestone
v6.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mikeykoelbl @bmuschko @cdancy @orzeh