Skip to content

Update NamespaceFilter description in the ConsoleExternalLogLink #385

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Jul 18, 2019
Merged

Update NamespaceFilter description in the ConsoleExternalLogLink #385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions console/v1/types_console_external_log_links.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ type ConsoleExternalLogLinkSpec struct {
// 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 set of namespaces (e.g., `/^openshift-/g`). If not specified, links will
// be displayed for all the namespaces.
// matching set of namespaces (e.g., `^openshift-`). The string is converted
Copy link
Contributor

@benjaminapetersen benjaminapetersen Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad to make a mess of this one 😬

Aiming to make it clear that there is a little potential gotcha hidden in here:

var str = "openshift-is-cool-openshift-is-fun"

var regex7 = "^(openshift|openshift-.*|kube-.*|kubernetes-.*|default)$"
// the user cann't provide this one, it is a regex literal.  in our definition we 
// have declared NamespaceFilter a string, so it will be passed to JS as a string
var regex8 = /^(openshift|openshift-.*|kube-.*|kubernetes-.*|default)$/
// That probably means it will end up looking like this, which will not work:
var regex9 = "/^(openshift|openshift-.*|kube-.*|kubernetes-.*|default)$/"

console.log("regex7", str.search(regex7)); // returns index 0
console.log("regex8", str.search(regex8)); // can't provide a regex literal, will convert to regex9 style
console.log("regex9", str.search(regex9)); // this will always return -1

Copy link
Contributor

@benjaminapetersen benjaminapetersen Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett @jhadvig this makes it more clear what I was trying to get at. I successfully pulled a gotcha on myself even :)

Copy link
Contributor

@benjaminapetersen benjaminapetersen Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: We should probably imply, do not use the / at all.

Copy link
Contributor

@benjaminapetersen benjaminapetersen Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I won't hold on it tho)

// into a regular expression using the JavaScript RegExp constructor.
// If not specified, links will be displayed for all the namespaces.
// + optional
NamespaceFilter string `json:"namespaceFilter,omitempty"`
}
2 changes: 1 addition & 1 deletion console/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.