Skip to content
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.

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

formatter: escape percent sign in response format #19383

Closed
wants to merge 3 commits into from

Conversation

aidanhahn
Copy link
Contributor

@aidanhahn aidanhahn commented Jan 4, 2022

Add a case in the SubstitutionFormatter to replace '%%' with '%'.
This allows users to escape % in their configurations, fixing #15159.

Signed-off-by: Aidan Hahn aidanhahn@datawire.io

Commit Message: formatter: escape percent sign in response format
Additional Description:
Risk Level: Low
Testing: Unit testing
Docs Changes:
Release Notes:
Platform Specific Features:
Fixes: #15159

Signed-off-by: Aidan Hahn <aidanhahn@datawire.io>
@repokitteh-read-only
Copy link

Hi @aidanhahn, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19383 was opened by aidanhahn.

see: more, trace.

@jmarantz
Copy link
Contributor

jmarantz commented Jan 5, 2022

/assign-from @envoyproxy/first-pass-reviewers

Can you clean up the description (remove optional fields and hints)?

@repokitteh-read-only
Copy link

@envoyproxy/first-pass-reviewers assignee is @jmarantz

🐱

Caused by: a #19383 (comment) was created by @jmarantz.

see: more, trace.

@jmarantz
Copy link
Contributor

jmarantz commented Jan 5, 2022

Also, please don't force-push. More detail about this will be added to the developers doc in #19381

@aidanhahn
Copy link
Contributor Author

I have //test/integration:buffer_accounting_integration_test passing in my local environment. Could it be a flake? Is there a way I can re-trigger the CI tests?

@rojkov
Copy link
Member

rojkov commented Jan 7, 2022

To rerun failed checks without making changes just add this line in a comment:

/retest

More info on this is in https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#triggering-ci-re-run-without-making-changes

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19383 (comment) was created by @rojkov.

see: more, trace.

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

I found a few superficial nits but I think this needs a look from someone who's worked with the formatters before.

test/common/formatter/substitution_formatter_test.cc Outdated Show resolved Hide resolved
source/common/formatter/substitution_formatter.cc Outdated Show resolved Hide resolved
@aidanhahn
Copy link
Contributor Author

Previously you mentioned I should not force push, is it acceptable to force push in order to fix DCO?

@jmarantz
Copy link
Contributor

jmarantz commented Jan 7, 2022

That's the only reason to do it, but generally you should not have DCO problems if you set up the right git hooks.

The main reason it's OK to force-push to fix DCO is that usually happens exactly once, before anyone has started reviewing. If you force-push after people start reviewing it wipes out their comments.

So my suggestion is to force-push this time, but install the git hooks so this never needs to happen again, and we'll live with my precious review being wiped :)

Signed-off-by: Aidan Hahn <aidanhahn@datawire.io>
@aidanhahn
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19383 (comment) was created by @aidanhahn.

see: more, trace.

Signed-off-by: Aidan Hahn <aidanhahn@datawire.io>
@jmarantz
Copy link
Contributor

/wait

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 11, 2022
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escape percent sign in log/custom response format
5 participants