Add ConsoleExternalLogLink type #380
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/M
console/v1/types.go
Outdated
| } | ||
|
|
||
| // Represents ability to provide custom pod log links. | ||
| type LogLink struct { |
There was a problem hiding this comment.
Wasn't really sure if w want to have it in the types.go or directly types_console_log_links.go, but since it was similar to the Link struct, I've put it here.
There was a problem hiding this comment.
Or maybe:
LogLinkTemplatePodLogLinkPodLogLinkTemplate
Wasn't sure regarding the name here..
There was a problem hiding this comment.
I put Link here as it is shared among several of the other resources. I don't think this one will be shared, so it may be best in types_console_log_links.go.
The naming is tricky agree. Since this one is embedded, it doesn't matter quite as much, but it would be good if it was clear.
There was a problem hiding this comment.
This will eventually be generic for other types, so I would avoid using "pod" in the name.
console/v1/register.go
Outdated
| func addKnownTypes(scheme *runtime.Scheme) error { | ||
| scheme.AddKnownTypes(GroupVersion, | ||
| &ConsoleLink{}, | ||
| &ConsoleLogLink{}, |
There was a problem hiding this comment.
I mentioned on slack, not sure I'm happy with ConsoleLink and ConsoleLogLink. The names are very similar, and the word Log doesn't really clarify intent.
The new one is something more like ConsoleExternalServiceLink. (Service may also be an overloaded term, however). ConsoleLink is really for simple menu links. The new link is for much more contextual link generation that could be for logs, monitoring, probably other kinds of services.
There was a problem hiding this comment.
This is so specific to logs, I'm not sure "External Service Link" is good. Do we have any other specific kinds of external services we're planning to add? Without knowing what will come later, it's difficult to say whether the properties here will work for other kinds of services. It's going to be hard to add a different kind of external service later if we need completely different properties.
I agree the names are too close. Maybe ConsoleExternalLogLink is better?
There was a problem hiding this comment.
From resourceName, containerName, resourceNamespace, podLabels it seems like this is pretty generic. If we had another use case arise, would we expand this further, or would we rather create a third CRD? I'd lean towards just expanding available parameters here. Otherwise, we would end up with ConsoleLink, ConsoleLogLink, ConsoleFooLink, seems like potential for more confusion.
Put another way, do we want to make it clear that we do not desire to add any other resources, but would rather expand this one (if future additional needs arise)?
|
cc @bparees |
console/v1/types.go
Outdated
| // - podLabels | ||
| // | ||
| // e.g., https://example.com/logs?resourceName=\${resourceName}&containerName=\${containerName}&resourceNamespace=\${resourceNamespace}&podLabels=\${podLabels} | ||
| HrefTemplate string `json:"href"` |
There was a problem hiding this comment.
| HrefTemplate string `json:"href"` | |
| HrefTemplate string `json:"hrefTemplate"` |
console/v1/types.go
Outdated
| // text is the display text for the link | ||
| Text string `json:"text"` | ||
| // Absolute secure URL (must use https) for the log link including variables | ||
| // to be replaced. Supported variables, which must exactly match, include: |
There was a problem hiding this comment.
We need to be more explicit about the exact format of the variables in the template. The example hints at it, but better to make it very clear.
console/v1/types.go
Outdated
| } | ||
|
|
||
| // Represents ability to provide custom pod log links. | ||
| type LogLink struct { |
There was a problem hiding this comment.
If this isn't being reused, I'd just inline it in ConsoleLogLinkSpec
| type ConsoleLogLinkSpec struct { | ||
| LogLink `json:",inline"` | ||
| // An optional, regular expression used to restrict a log link to a matching | ||
| // namespace (e.g., ^openshift-). |
There was a problem hiding this comment.
You should mention that the links will be displayed for all namespaces if not specified
There was a problem hiding this comment.
Doc string should start with the JSON field name
console/v1/types.go
Outdated
| type LogLink struct { | ||
| // text is the display text for the link | ||
| Text string `json:"text"` | ||
| // Absolute secure URL (must use https) for the log link including variables |
There was a problem hiding this comment.
Doc string should start with the JSON field name
console/v1/types.go
Outdated
| Href string `json:"href"` | ||
| } | ||
|
|
||
| // Represents ability to provide custom pod log links. |
There was a problem hiding this comment.
Doc string should start with the struct name
jhadvig
force-pushed
the
podlinks
branch
2 times, most recently
from
90d0c79 to
77aaea0
Compare
|
@spadgett @benjaminapetersen I've udpated the PR addressing your comments. |
|
@jhadvig yup, whatever we land on has to match across these PRs. |
| // matching namespace (e.g., ^openshift-). If not specified, links will be | ||
| // displayed for all the namespaces. | ||
| // + optional | ||
| NamespaceFilter string `json:"namespaceFilter,omitempty"` |
There was a problem hiding this comment.
Do i always get the k8s log link, and then if the namespacefilter matches, i get an additional (external) log link?
What happens if i have multiple ConsoleExternalLogLink objects, for an overlapping set of namespacefilters?
There was a problem hiding this comment.
Do i always get the k8s log link, and then if the namespacefilter matches, i get an additional (external) log link?
I believe that you only get the link that matches the namespaceFilter
What happens if i have multiple ConsoleExternalLogLink objects, for an overlapping set of namespacefilters?
Not really sure with this one, @rhamilto cau you assist on this one ?
There was a problem hiding this comment.
What happens if i have multiple ConsoleExternalLogLink objects, for an overlapping set of namespacefilters?
You get multiple links. Note the filter is optional. You'd get as many links as you have instances that aren't filtered in addition to any instances that match the filter.
There was a problem hiding this comment.
ok. i'm a little surprised we remove the k8s log link when a custom log link exists, seems like that could still be useful?
But i assume you guys have been through this w/ users/customers/use-cases?
There was a problem hiding this comment.
jhadvig
force-pushed
the
podlinks
branch
3 times, most recently
from
a8ef81d to
d11fd2f
Compare
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
| // text is the display text for the link | ||
| Text string `json:"text"` | ||
| // hrefTemplate is an absolute secure URL (must use https) for the log link including | ||
| // variables to be replaced. Supported variables, which must exactly match, include: |
There was a problem hiding this comment.
We should say something like:
Variables are specified in the URL with the format
${variableName}, for intsance,${containerName}.
I'd probably take out which must exactly match. That seems self-evident.
| // - podLabels - JSON representation of labels matching the pod with the logs | ||
| // - e.g. `{"key1":"value1","key2":"value2"}` | ||
| // | ||
| // Resource might be a pod, build, buildconfig, deployment or deploymentconfig. |
There was a problem hiding this comment.
For now, resource is only pods.
| Spec ConsoleExternalLogLinkSpec `json:"spec"` | ||
| } | ||
|
|
||
| // ConsoleExternalLogLinkSpec is the desired console pod log link configuration. |
There was a problem hiding this comment.
I would state the link will appear on the logs tab of the pod details page.
There was a problem hiding this comment.
Something like:ConsoleExternalLogLinkSpec is the desired log link configuration. The log link will appear on the logs tab of the pod detail page.
| // +genclient:nonNamespaced | ||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // ConsoleExternalLogLink is an extension for customizing OpenShift web console pod log links. |
There was a problem hiding this comment.
can prob drop pod from ...customizing OpenShift web console pod log... since we renamed.
| Spec ConsoleExternalLogLinkSpec `json:"spec"` | ||
| } | ||
|
|
||
| // ConsoleExternalLogLinkSpec is the desired console pod log link configuration. |
There was a problem hiding this comment.
Can prob drop pod from ...desired console pod log... since we renamed.
| Text string `json:"text"` | ||
| // hrefTemplate is an absolute secure URL (must use https) for the log link including | ||
| // variables to be replaced. Supported variables, which must exactly match, include: | ||
| // - resourceName - name of the resource which containes the logs |
There was a problem hiding this comment.
We should probably include the ${ } syntax in each of these:
// - ${resourceName} - name of the resource which containes the logs
// ...
To make it explicit (just like the example).
| // e.g., https://example.com/logs?resourceName=${resourceName}&containerName=${containerName}&resourceNamespace=${resourceNamespace}&podLabels=${podLabels} | ||
| HrefTemplate string `json:"hrefTemplate"` | ||
| // namespaceFilter is a regular expression used to restrict a log link to a | ||
| // matching namespace (e.g., ^openshift-). If not specified, links will be |
There was a problem hiding this comment.
...to a matching set of namespaces..., it may be a set of one, or many.
There was a problem hiding this comment.
I think the example has to be:
/^openshift-/g
If you just provide ^openshift-, our code isn't going to treat that like a regex
https://github.com/openshift/console/pull/1892/files#r304608136
There was a problem hiding this comment.
@benjaminapetersen @jhadvig I don't think this comment is right... It will be treated a string:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search
A regular expression object. If a non-RegExp object obj is passed, it is implicitly converted to a RegExp by using new RegExp(obj).
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
There was a problem hiding this comment.
My mistake! We can revise that.
|
@spadgett @benjaminapetersen thanks for the review and suggestions ! Much appreciated. |
|
/approve |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, jhadvig, spadgett 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 |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
We should open a follow on PR to address #380 (comment) Would be good to test with @rhamilto's changes to verify |
|
Yeah, confirmed the original example works If we wanted to be really clear, we could say it's a string converted into a regular expression using the JavaScript The updated example does not work: |
|
Tested a handful: var str = "openshift-is-cool-openshift-is-fun"
var regex1 = "openshift-"
var regex2 = "^openshift-"
var regex3 = "/^openshift-/g"
var regex4 = /^openshift-/g
console.log("regex1", str.search(regex1)); // returns index 0
console.log("regex2", str.search(regex2)); // returns index 0
console.log("regex3", str.search(regex3)); // returns index -1
console.log("regex4", str.search(regex4)); // returns index 0So since this is coming from |
7 participants
Story: https://jira.coreos.com/browse/CONSOLE-1546
Based on the changes in https://github.com/openshift/console-operator/pull/246/files
Adding comments to open discussion regarding the naming. Will add generated code afterwards.
/assign @spadgett @benjaminapetersen
@rhamilto fyi