logs storage service and RPC definitions #277
Conversation
tekton-robot
added
the
needs-ok-to-test
|
Hi @sayan-biswas. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tekton-robot
added
the
size/XXL
|
/assign @adambkaplan |
sayan-biswas
force-pushed
the
logs-api
branch
from
a144cd5 to
8644014
Compare
|
/ok-to-test |
tekton-robot
added
ok-to-test
| resultsClient: resultsClient, | ||
| logsClient: logs.Get(ctx), |
There was a problem hiding this comment.
Non-blocking observation - some things we are wiring through the Context object, others we are not. Unsure if/how we can establish some consistency here.
There was a problem hiding this comment.
All the other clients and informers used in Knative are injected in the controller context (as in other tekton controllers). I guess injected client is the consistent way when working with Knative.
There was a problem hiding this comment.
Yes. We can try moving to knative/pkg but it's okay for now. We did this with the Trigger's EventListener sink HTTP server. The first implementation wasn't Knative. I will look into doing this with GRPC.
| if configFile.LOGS_API { | ||
| v1alpha2pb.RegisterLogsServer(s, v1a2) | ||
| } |
There was a problem hiding this comment.
This makes sense to me - if we want logs to be an opt-in feature, disable the API completely if it is not available.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
approved
sayan-biswas
force-pushed
the
logs-api
branch
from
8644014 to
0682e34
Compare
tekton-robot
removed
the
needs-rebase
cmd/api/README.md
Outdated
| | TLS_HOSTNAME_OVERRIDE| Override the hostname used to serve TLS. This should not be set (or set to the empty string) in production environments. | results.tekton.dev | | ||
| | TLS_PATH | Path to TLS files | /etc/tls | | ||
| | LOGS_API | Enable logs storage service | false (default) | | ||
| | Environment Variable | Description | Example | |
sayan-biswas
force-pushed
the
logs-api
branch
from
0682e34 to
2fbe21f
Compare
tekton-robot
added
the
needs-rebase
sayan-biswas
force-pushed
the
logs-api
branch
from
2fbe21f to
ec8eed8
Compare
tekton-robot
removed
the
needs-rebase
sayan-biswas
force-pushed
the
logs-api
branch
from
ec8eed8 to
0fcd019
Compare
|
/assign @dibyom /cc @alan-ghelardi Discussed with @sayan-biswas - I closed #258, and instead of one big PR we think it's better to land it in pieces. This adds logs as an optional feature on the apiserver. If logs are disabled, the APIs are not served. |
|
@adambkaplan: GitHub didn't allow me to request PR reviews from the following users: alan-ghelardi. Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Awesome, although I cannot approve yet. |
tekton-robot
added
the
kind/feature
This PR separates the service definition of logs from the results in the Proto file. Also the logs service registration is made optional at the API server and the watcher can negotiate the availability of logs service through a reflection client and enable/disable sending logs.