Cleanup receive adapter main code.#2016
Conversation
knative-prow-robot
added
the
size/L
lionelvillard
changed the title
lionelvillard
force-pushed
the
cleanup_adapters
branch
from
a6c764f to
9232f7a
Compare
|
/assign @evankanderson |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
/approve
/lgtm
/hold
A few comments, mostly around the removal of TODOs that I'm not sure if we want to clean up.
Feel free to remove the hold and let this submit if the TODOs are obsolete and you don't feel the need to make other fixes.
| LoggingConfigJson string `envconfig:"K_LOGGING_CONFIG" required:"true"` | ||
| } | ||
|
|
||
| // TODO: the controller should take the list of GVR |
There was a problem hiding this comment.
It looks like we lost this TODO; it looks like there's a dynamic list in the current code. I'm not sure what the original intent of this TODO was, though. Maybe it's obsolete?
There was a problem hiding this comment.
This is being tracked here #1660 instead of the TODO. Is that alright?
There was a problem hiding this comment.
I think that an issue is IMO better instead of a TODO, @lionelvillard
|
|
||
| var env envConfig | ||
| err := envconfig.Process("", &env) | ||
| if err != nil { |
There was a problem hiding this comment.
Convert this to the 3-line version?
if err := envconfig.Process("", &env); err != nil {
log.Fatalf(...)
}(I also don't think panic's stack trace will help here.)
| package adapter | ||
|
|
||
| // EnvConfig is the minimal set of configuration parameters | ||
| // source adapters must support |
There was a problem hiding this comment.
Is this a "must support" or "a common set of useful environment variables"?
There was a problem hiding this comment.
The controllers provide these variables and the adapters should support them. Changed
| "knative.dev/pkg/source" | ||
| ) | ||
|
|
||
| // TODO: this should be a k8s cron. |
| ) | ||
|
|
||
| // Initialize cloudevent client | ||
| func (a *Adapter) initClient() error { |
There was a problem hiding this comment.
+1 to removing init methods where possible.
(no action needed)
knative-prow-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, lionelvillard The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
/hold cancel |
knative-prow-robot
removed
the
do-not-merge/hold
|
/lgtm |
6 participants
Helps with #1989
Proposed Changes