-
Notifications
You must be signed in to change notification settings - Fork 22
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
KIM Integration - instances on runtimes
endpoint decorated with Runtime CR
#1061
KIM Integration - instances on runtimes
endpoint decorated with Runtime CR
#1061
Conversation
Add one of following labels |
0b65178
to
bf91abe
Compare
f997c61
to
63235fe
Compare
internal/runtime/handler.go
Outdated
Namespace: runtimeNamespaceName, | ||
Name: runtimeResourceName, | ||
}, runtimeResourceObject) | ||
if err != nil { |
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 in case of "Not Found" error we will have a problem 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.
If there is no runtime resource, let's do not throw any error, just leave the field empty.
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 log Warning and return empty result.
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.
handle NotFoundError -> log.Info, other error log as warn
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.
In the next commit/push.
internal/runtime/handler.go
Outdated
if err != nil { | ||
h.logger.Warn(fmt.Sprintf("unable to set optional attributes: %s", err.Error())) | ||
httputil.WriteErrorResponse(w, http.StatusInternalServerError, err) | ||
return | ||
} | ||
|
||
if runtimeResourceConfig && dto.RuntimeID != "" && instanceDrivenByKimOnly { |
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 the instanceDrivenByKimOnly
in not necessary here, I'd like to have a possibility to use kcp cli to get the runtime even if it is driven by Provisioner (in the phase when Provisioner is called and runtime resource is created).
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.
In the next commit/push.
4eb008f
to
1c82851
Compare
pass necessary data to handler barebones kim config moved to broker package change package reference change package reference again redo reredo removing unused functions fixes to fixes initial stubs for handler tests simplified tests setup handling errors from provisioner (in a nasty way) dto extended and set if RuntimeResource fetched fake client extended, remarks and todos added before test implementation suite corrected no scheme used and make fix run let focus on one test case failing new test cases first passing test case (out of new ones) test cases added error handling for nested fields fix for signature setting workers in the fixture additional assertion review remark and conflict resolved log level depending on k8s client error
9c02283
to
8d52691
Compare
Description
KEB endpoint
runtimes
is used bykcp
CLI e.g. to get gardener configuration information (using--gardener-config
switch), and this information is used quite often to assert in SKR tests. Getting gardener configuration requires communication with provisioner, so in case of KIM driven instances is pointless.We need the way to base assertions on the spec fields of Runtime CR.
Changes proposed in this pull request:
runtime-config
--gardener-config
flag we process instance not known to provisionerRelated issue(s)
#791
#905