Hybrid Cloud Environment API#87
Conversation
|
The latest Buf updates on your PR. Results from workflow Pull Request / linting (pull_request).
|
This comment has been minimized.
This comment has been minimized.
mikheillomidzeq
left a comment
mikheillomidzeq
left a comment
There was a problem hiding this comment.
Nice!
Left some comments
| optional string qdrant_cluster_manager_version = 19; | ||
| string created_by = 20; | ||
| optional string log_level = 21; | ||
| repeated qdrant.cloud.cluster.v1.Toleration tolerations = 22; |
There was a problem hiding this comment.
I am using Toleration from cluster package - should we move it to common package, given that is now used by two APIs? cc @Robert-Stam
There was a problem hiding this comment.
Good idea, however it’s a breaking change in the public api, so we need to check if TerraForm provider is using this field, because that’s already in production
There was a problem hiding this comment.
Good idea, however it’s a breaking change in the public api, so we need to check if TerraForm provider is using this field, because that’s already in production
we are not changing API, request/response schemas remain the same. What changes though is the generated client code, but that would mean that we need to update TerraForm provider to use the new code version, right?
There was a problem hiding this comment.
Changing the location/package of the message (of types) is from a gRPC PoV a hard breaking change afaik (in fact it's another type). If/how people the TF provider updates is not in our control, so imo we should stick what we have.
There was a problem hiding this comment.
Changing the location/package of the message (of types) is from a gRPC PoV a hard breaking change afaik (in fact it's another type). If/how people the TF provider updates is not in our control, so imo we should stick what we have.
Ok, should I then use in this PR k8s.io.api.core.v1.Toleration type instead, to avoid unneeded dependency between cluster and hybrid env API?
There was a problem hiding this comment.
That would be Ok, however K8s types are not in the buf registry, so I would keep as-is (re-use of cluster).
In the end - based on the doc - this is the toleration for the clusters, so not a bad thing.
pedjak
force-pushed
the
enh/predrag/hybrid-cloud-service
branch
from
358079a to
5030d2d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
areina
left a comment
areina
left a comment
There was a problem hiding this comment.
I haven't finished the full review, submitting it to talk about it during our call.
pedjak
force-pushed
the
enh/predrag/hybrid-cloud-service
branch
from
dc626ad to
af9d3f2
Compare
This comment has been minimized.
This comment has been minimized.
areina
left a comment
areina
left a comment
There was a problem hiding this comment.
minor details, it's looking great and way simpler than before 👏
This comment has been minimized.
This comment has been minimized.
pedjak
force-pushed
the
enh/predrag/hybrid-cloud-service
branch
from
fede129 to
326077d
Compare
|
LGTM |
pedjak
force-pushed
the
enh/predrag/hybrid-cloud-service
branch
from
28f0169 to
23b0c08
Compare
pedjak
force-pushed
the
enh/predrag/hybrid-cloud-service
branch
from
23b0c08 to
e30c32e
Compare
5 participants
Part of CRC-320