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

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

Update NamespaceFilter description in the ConsoleExternalLogLink #385

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

Conversation

@jhadvig
Copy link
Member

@jhadvig jhadvig commented Jul 17, 2019

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 17, 2019
spadgett
spadgett approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

thanks @jhadvig. i should have caught this on my original review

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019
@spadgett
Copy link
Member

spadgett commented Jul 17, 2019

/assign @bparees

benjaminapetersen
benjaminapetersen reviewed Jul 17, 2019
View reviewed changes
console/v1/types_console_external_log_links.go
// 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)

@bparees
Copy link
Contributor

bparees commented Jul 17, 2019

/approve

i'm indifferent about clarifying that / should not be used.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 17, 2019

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019
@bparees
Copy link
Contributor

bparees commented Jul 17, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2019
@bparees
Copy link
Contributor

bparees commented Jul 17, 2019

(remove the hold at will)

@jhadvig
Copy link
Member Author

jhadvig commented Jul 17, 2019

@spadgett

@spadgett spadgett mentioned this pull request Jul 18, 2019
Merged
@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Jul 18, 2019

No need to hold on my comment so lets see if I can

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2019
@openshift-merge-robot openshift-merge-robot merged commit 86a969f into openshift:master Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett approved these changes

+1 more reviewer

@benjaminapetersen benjaminapetersen benjaminapetersen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@spadgett spadgett

@bparees bparees

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jhadvig @spadgett @bparees @openshift-ci-robot @benjaminapetersen @openshift-merge-robot