Sitelet https://github.com/dotnet/Docker.DotNet/pull/590
Skip to content

Handle Reponse Error when building image - #590

Merged
galvesribeiro merged 6 commits into
dotnet:masterfrom
aDisplayName:pr/555-handle-error-when-building-image
Sep 23, 2022
Merged

galvesribeiro merged 6 commits into
dotnet:masterfrom
aDisplayName:pr/555-handle-error-when-building-image

Conversation

@aDisplayName

@aDisplayName aDisplayName commented Sep 21, 2022 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

#555

@aDisplayName

Copy link
Copy Markdown
Contributor Author

@HofmeisterAn , can you take a look?

@HofmeisterAn HofmeisterAn left a comment

Copy link
Copy Markdown
Contributor

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

dnfadmin commented Sep 22, 2022 •

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@aDisplayName

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@HofmeisterAn , The test has been added.

@aDisplayName

Copy link
Copy Markdown
Contributor Author

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

@HofmeisterAn HofmeisterAn left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread test/Docker.DotNet.Tests/IImageOperationsTests.cs Outdated
@aDisplayName

Copy link
Copy Markdown
Contributor Author

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

@aDisplayName

aDisplayName commented Sep 23, 2022 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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

HofmeisterAn commented Sep 23, 2022 •

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Member

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

Thank you!

@HofmeisterAn

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@HofmeisterAn

Copy link
Copy Markdown
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