-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
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 wonder if we can add more to this documentation. I am not sure
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 added some suggestions for improvements.
Also, please add some issue reference to the commit for context.
d9633e7
to
32907c7
Compare
32907c7
to
6cfada1
Compare
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 think we could mention locksmithd
and not recommend disabling updates for nodes, otherwise looks good. Nice work @surajssd.
6cfada1
to
c044b5e
Compare
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.
Added some comments.
For some reason I can't comment on threads so I'll answer here.
Well, I think treating components as cluster-agnostic, even when the compatibility is not guaranteed is something valuable. What do you think @iaguis?
In general I don't think we should go out of our way to handle non-Lokomotive clusters, trying to keep that kind of compatibility can lead to expectations so I'd rather be opinionated here.
However, something we should support is managed k8s platforms like AKS, since even though they're managed k8s, they're still Lokomotive clusters.
c044b5e
to
b986df1
Compare
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.
It reads very nicely, I like it a lot. Left 2 optional suggestions.
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
b986df1
to
e6e60f4
Compare
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.
👌
These suggestions have been incorporated.
Fixes: #1260