Skip to content

Missing quote in match example#928

Merged
linsun merged 4 commits intoistio:masterfrom
esnible:quote-example
Jun 12, 2019
Merged

Missing quote in match example#928
linsun merged 4 commits intoistio:masterfrom
esnible:quote-example

Conversation

@esnible
Copy link
Copy Markdown
Contributor

@esnible esnible commented May 22, 2019

Resolves #927

@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 22, 2019
@istio-testing istio-testing requested review from mandarjog and wattli May 22, 2019 10:49
@esnible esnible requested a review from geeknoid May 22, 2019 10:49
Copy link
Copy Markdown
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

/lgtm

@istio-testing
Copy link
Copy Markdown
Collaborator

@linsun: changing LGTM is restricted to assignees, and assigning you to the PR failed.

Details

In response to this:

/lgtm

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.

@linsun linsun self-assigned this May 28, 2019
@linsun
Copy link
Copy Markdown
Member

linsun commented May 28, 2019

/lgtm

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented May 29, 2019

/test api-presubmit

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented May 29, 2019

@geeknoid any idea why this is failing presubmit?

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Jun 5, 2019

/test api-presubmit

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Jun 5, 2019

@geeknoid any idea why this is failing presubmit?

@linsun
Copy link
Copy Markdown
Member

linsun commented Jun 10, 2019

here is the failing log, still not sure why it failed tho @geeknoid @howardjohn @fpesce. It certainly complained about the change from PR.

I0605 21:16:24.752] --- a/policy/v1beta1/cfg.pb.go
I0605 21:16:24.752] +++ b/policy/v1beta1/cfg.pb.go
I0605 21:16:24.752] @@ -306,7 +306,7 @@ type Rule struct {
I0605 21:16:24.752]  	//
I0605 21:16:24.752]  	// * an empty match evaluates to `true`
I0605 21:16:24.752]  	// * `true`, a boolean literal; a rule with this match will always be executed
I0605 21:16:24.752] -	// * `match(destination.service.host, "ratings.*)` selects any request targeting a service whose
I0605 21:16:24.753] +	// * `match(destination.service.host, "ratings.*")` selects any request targeting a service whose
I0605 21:16:24.753]  	// name starts with "ratings"
I0605 21:16:24.753]  	// * `attr1 == "20" && attr2 == "30"` logical AND, OR, and NOT are also available
I0605 21:16:24.753]  	Match string `protobuf:"bytes,1,opt,name=match,proto3" json:"match,omitempty"`
I0605 21:16:24.753] diff --git a/policy/v1beta1/istio.policy.v1beta1.pb.html b/policy/v1beta1/istio.policy.v1beta1.pb.html
I0605 21:16:24.753] index eae6280..6b3dd9b 100644
I0605 21:16:24.753] --- a/policy/v1beta1/istio.policy.v1beta1.pb.html
I0605 21:16:24.753] +++ b/policy/v1beta1/istio.policy.v1beta1.pb.html
I0605 21:16:24.753] @@ -1115,7 +1115,7 @@ if the match evaluates to true.</p>
I0605 21:16:24.753]  <ul>
I0605 21:16:24.753]  <li>an empty match evaluates to <code>true</code></li>
I0605 21:16:24.754]  <li><code>true</code>, a boolean literal; a rule with this match will always be executed</li>
I0605 21:16:24.754] -<li><code>match(destination.service.host, &quot;ratings.*)</code> selects any request targeting a service whose
I0605 21:16:24.754] +<li><code>match(destination.service.host, &quot;ratings.*&quot;)</code> selects any request targeting a service whose
I0605 21:16:24.754]  name starts with &ldquo;ratings&rdquo;</li>
I0605 21:16:24.754]  <li><code>attr1 == &quot;20&quot; &amp;&amp; attr2 == &quot;30&quot;</code> logical AND, OR, and NOT are also available</li>
I0605 21:16:24.754]  </ul>
I0605 21:16:24.754] Repo has unstaged changes. Re-run ./scripts/generate-protos.sh or make proto-commit
E0605 21:16:24.754] Build failed
I0605 21:16:24.754] process 580 exited with code 1 after 1.6m
E0605 21:16:24.755] FAIL: api-presubmit

@howardjohn
Copy link
Copy Markdown
Member

@esnible

You need to run "make" before submitting the PR so that the (unforutnately) checked in generated files are correctly updated. Doing so will fix the CI failures.

From Martin on #795 (comment)

@googlebot
Copy link
Copy Markdown
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. labels Jun 11, 2019
@howardjohn
Copy link
Copy Markdown
Member

@esnible you somehow picked up some other changes in here, maybe a merge gone wrong

@esnible esnible changed the base branch from master to release-1.2 June 11, 2019 17:44
@esnible esnible changed the base branch from release-1.2 to master June 11, 2019 17:45
@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Jun 11, 2019

@howardjohn I ran make as requested. I rebased and pushed, causing Git to think I had changed 46 files. I changed the target away from master, then back, using the Github web UI. Seems to have fixed the problem. Github now shows exactly the intended changes, but the CLA fell off. I'll need a Googler to restore it manually.

@howardjohn howardjohn added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Jun 11, 2019
@googlebot
Copy link
Copy Markdown
Collaborator

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Jun 11, 2019

@linsun After my rebase your /lgtm fell off.

Copy link
Copy Markdown
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: esnible, linsun
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: greghanson

If they are not already assigned, you can assign the PR to them by writing /assign @greghanson in a comment when ready.

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

@linsun linsun merged commit 2f353a3 into istio:master Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typo in policy/v1beta1/cfg.proto

5 participants