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

ISystemOperations.AuthenticateAsync now returns AuthResponse. #496

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ISystemOperations.AuthenticateAsync now returns AuthResponse. #496

wants to merge 1 commit into from

Conversation

Emdot
Copy link
Contributor

@Emdot Emdot commented Feb 18, 2021

Fixes #493

Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM

@jterry75
Copy link
Contributor

@galvesribeiro - PTAL

Copy link
Member

@galvesribeiro galvesribeiro left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@galvesribeiro
Copy link
Member

Just one thing before we merge. Why is the tests failing? I'm out of my desk now and I can't check. Can you please fix it and push the update? Thanks!

@Emdot
Copy link
Contributor Author

Emdot commented Feb 19, 2021

It's failing MonitorEventsAsync_Succeeds. Running locally, it appears to be an intermittent failure.

@dgvives
Copy link
Contributor

dgvives commented Feb 20, 2021

@galvesribeiro @Emdot errors on StreamUtils.MonitorStreamAsync methods are not related to this changeset but to previously existing changes.

Previous ContainerOperations.GetContainerLogsAsync task cancellation by disposing stream had some pitfalls, as it was disposing a stream while a streamReader was consuming from it.

Should be working fine now, pushed to #487 .
@Emdot feel free to review/fix/improve/criticise this PR. There is integrity within your criteria

@Emdot
Copy link
Contributor Author

Emdot commented Feb 27, 2021

@galvesribeiro, can you retrigger the build?

@galvesribeiro
Copy link
Member

Just did. Same error as it seems... ☹️

@Emdot
Copy link
Contributor Author

Emdot commented Feb 27, 2021

I see that this is a known bug in the test case. "// On CI/CD Pipeline exception is thrown, not always"

I think there's more of a problem here than I'm willing to fix:

  1. It looks like the event streams are delayed. Some of the events from CreateImageAsync don't arrive until after MonitorEventsAsync starts, so they show up in _onJSONMessageCalled. Some of the events from TagImageAsync don't arrive until after the assertion.
  2. I can't be sure that that's what's happening, since stdio might be buffered too. Adding some Console.Out.Flush and Console.Error.Flush calls between commands can help check for that.
  3. This probably isn't what's going wrong in this case, but it's a bug anyway: Using MonitorEventsAsync this way has a race condition. Unless you await MonitorEventsAsync's Task, you can't guarantee that the monitoring has started, so you can miss messages. The fix would is to replace MonitorEventsAsync with something like StartEventMonitorAsync that returns a subscription.

Would you accept a PR to disable the broken test case, until someone fixes the underlying problems?

@Emdot
Copy link
Contributor Author

Emdot commented Feb 27, 2021

I wrote that up as #502.

@dgvives
Copy link
Contributor

dgvives commented Feb 27, 2021

@Emdot move the call to include a call to remove the image after tagging it, that should be enough in this case

Try to include some delay before cancelling task so that I/O operation finishes
Task.Delay(TimeSpan.FromSeconds(1))

@Emdot
Copy link
Contributor Author

Emdot commented Apr 4, 2021

I'm unwilling to fudge the tests to make them pass. If you fix the problematic code (as noted above) or disable the test, then this PR can proceed.

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.

Get token from AuthenticateAsync
4 participants