-
Notifications
You must be signed in to change notification settings - Fork 306
introduce watch api #35
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
Conversation
Let me know if you need some help with the k8s client template to make this less hacky. |
src/Watcher.cs
Outdated
|
||
public class Watcher<T> : IDisposable | ||
{ | ||
private readonly StreamReader _streamReader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use the underscore convention elsewhere, do we? I think we should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md
We use _camelCase for internal and private fields and use readonly where possibl
Actually, I use resharper out of box coding style formatter
https://www.jetbrains.com/resharper/features/code_formatting.html
public enum WatchEventType | ||
{ | ||
[EnumMember(Value = "ADDED")] Added, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not a csharp style expert, but it feels weird to have newlines between these values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really love go fmt
we might introduce something like https://github.com/dotnet/codeformatter and contribution guide. I will create another issue to track this.
src/Watcher.cs
Outdated
|
||
OnEvent?.Invoke(@event.Type, @event.Object); | ||
} | ||
catch (Exception e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't catching all exceptions an anti-pattern? Maybe just the parsing and IO exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi please have a look at my testcases which would better illustrate what I want to do.
- TestSuriveBadLine: the watch object will still alive when json parse error or exception from onevent. the
onerror
will be triggered - TestWatchServerDisconnect: the watch object stops as of transport level error. the
onerror
will be triggered
src/WatcherDelegatingHandler.cs
Outdated
|
||
namespace k8s | ||
{ | ||
internal class WatcherDelegatingHandler : DelegatingHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps document what this class does here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/WatcherDelegatingHandler.cs
Outdated
|
||
if (originResponse.IsSuccessStatusCode) | ||
{ | ||
if ($"{request.RequestUri.Query}".Contains("watch=true")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do better parsing, so that ?baywatch=true-ish
won't trigger this handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
this must be something I am missing, cloud you please referrer any full doc about watch in k8s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just saying that a user can attach random query strings (possibly accidentally, possibly on purpose) and this code will trigger a watch, even if a watch isn't actually being requested.
We should parse the query string into a map, and then check map['watch'] == 'true'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduce a dependency Microsoft.AspNetCore.WebUtilities
to parse the query. I am not sure you might consider it too heavy.
another approach is
if ($"&{request.RequestUri.Query}&".Contains("&watch=true&"))
|| ($"&{request.RequestUri.Query}&".Contains("&?watch=true&"))
seems too ugly
Some small-ish comments. Many thanks for the PR! |
travis ci stucks if I also tested on my Ubuntu 16.04 and Windows, seems a travis ci problem. |
LGTM. Thanks! |
fix #8
The API is like
since autorest generated code use
ReadAsString
to receive the data from api server, json stream would be hang.I tried to hack autorest first, but it is hard to change the template for k8s client.
The implementation used an HttpDelegatingHandler to rewrite the response and return first line to autorest client, then use WatchExt to create a watch object which interact with the replaced http response to get watch works.