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.

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

Handle Reponse Error when building image #590

Merged
merged 6 commits into from
Sep 23, 2022
Merged

Handle Reponse Error when building image #590

merged 6 commits into from
Sep 23, 2022

Conversation

aDisplayName
Copy link
Contributor

@aDisplayName aDisplayName commented Sep 21, 2022

MakeRequestForRawResponseAsync is used when calling IImageOperations.CreateImageAsync. But the response error from daemon was not handled. Add the error handling before return the result.

Fix issue #555

@aDisplayName
Copy link
Contributor Author

#555

@aDisplayName
Copy link
Contributor Author

@HofmeisterAn , can you take a look?

Copy link
Contributor

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

That is great. I stumbled across this issue too. Thanks for fixing it. Can you add a small test too?

@dnfadmin
Copy link

dnfadmin commented Sep 22, 2022

CLA assistant check
All CLA requirements met.

@aDisplayName
Copy link
Contributor Author

That is great. I stumbled across this issue too. Thanks for fixing it. Can you add a small test too?

I need to figure it out... It seems the existing tests were done against actual docker hub registry.

So instead, I tries a real non-existing repo, and finger crossed no one will create such repository for real in the future.

@aDisplayName
Copy link
Contributor Author

@HofmeisterAn , The test has been added.

@aDisplayName
Copy link
Contributor Author

@galvesribeiro , can you help to take a look as well? Thank you.

Copy link
Contributor

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

LGTM. @galvesribeiro It makes sense to merge it. I know a couple of devs that run into this issue.

test/Docker.DotNet.Tests/IImageOperationsTests.cs Outdated Show resolved Hide resolved
@aDisplayName
Copy link
Contributor Author

Alright, so who is the "maintainer" that can approve for the workflow?

@aDisplayName
Copy link
Contributor Author

aDisplayName commented Sep 23, 2022

Okay, it looks like by enabling the error handling and throwing out the exception, some of the other tests utilizing the bug have failed. .... Let me figure out whether should we disable those tests:

Docker.DotNet.Tests.ISystemOperationsTests.MonitorEventsAsync_Succeeds();
Docker.DotNet.Tests.ISystemOperationsTests.MonitorEventsFiltered_Succeeds();

@HofmeisterAn
Copy link
Contributor

@galvesribeiro has permissions to merge it. Let's wait until he caught up. No one is maintaining the project full time.

Let me figure out whether should we disable those tests.

We should fix the tests instead. I can take a look at it in the next days if you need some help. Just let me know.

@aDisplayName
Copy link
Contributor Author

@galvesribeiro has permissions to merge it. Let's wait until he caught up. No one is maintaining the project full time.

Let me figure out whether should we disable those tests.

We should fix the tests instead. I can take a look at it in the next days if you need some help. Just let me know.

I've updated those tests.
Docker.DotNet.Tests.ISystemOperationsTests.MonitorEventsAsync_Succeeds(); seems kind of fishy at the moment.

@galvesribeiro galvesribeiro merged commit 4119439 into dotnet:master Sep 23, 2022
@HofmeisterAn
Copy link
Contributor

HofmeisterAn commented Sep 23, 2022

@galvesribeiro That was a bit to early 😀, I was just replying - sorry.

I think the last commit is wrong. We should revert it and remove CreateImageAsync in both tests. They make no sense. The image does not exist and cannot pulled or imported. IMO the right fix would be (in addition the test class had way to many unnecessary changes):

diff --git a/test/Docker.DotNet.Tests/ISystemOperations.Tests.cs b/test/Docker.DotNet.Tests/ISystemOperations.Tests.cs
index 0157d54..fea6c01 100644
--- a/test/Docker.DotNet.Tests/ISystemOperations.Tests.cs
+++ b/test/Docker.DotNet.Tests/ISystemOperations.Tests.cs
@@ -111,8 +111,6 @@ namespace Docker.DotNet.Tests
                 progressMessage,
                 cts.Token);
 
-            await _dockerClient.Images.CreateImageAsync(new ImagesCreateParameters { FromImage = $"{_repositoryName}:{_tag}" }, null, progressJSONMessage, _cts.Token);
-
             await _dockerClient.Images.TagImageAsync($"{_repositoryName}:{_tag}", new ImageTagParameters { RepositoryName = _repositoryName, Tag = newTag }, _cts.Token);
 
             await _dockerClient.Images.DeleteImageAsync(
@@ -267,8 +265,6 @@ namespace Docker.DotNet.Tests
             using var cts = CancellationTokenSource.CreateLinkedTokenSource(_cts.Token);
             var task = Task.Run(() => _dockerClient.System.MonitorEventsAsync(eventsParams, progress, cts.Token));
 
-            await _dockerClient.Images.CreateImageAsync(new ImagesCreateParameters { FromImage = $"{_repositoryName}:{_tag}" }, null, new Progress<JSONMessage>());
-
             await _dockerClient.Images.TagImageAsync($"{_repositoryName}:{_tag}", new ImageTagParameters { RepositoryName = _repositoryName, Tag = newTag });
             await _dockerClient.Images.DeleteImageAsync($"{_repositoryName}:{newTag}", new ImageDeleteParameters());

Should I create another pull request?

@galvesribeiro
Copy link
Member

No problem. Can you create another PR and submit the fix?

Thank you!

@HofmeisterAn
Copy link
Contributor

No problem. Can you create another PR and submit the fix?

Thank you!

Yep, I'll do. Give me a few minutes.

@aDisplayName
Copy link
Contributor Author

@HofmeisterAn , I believe my last commit has already changed those non-existing image to the well-known 'hello-world:latest'. Or are you talking about other places?

@aDisplayName
Copy link
Contributor Author

@galvesribeiro @HofmeisterAn , would any of you have some estimation when the next release will be triggered?

@HofmeisterAn
Copy link
Contributor

Sorry, cannot help here. Waiting for a release as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants