Skip to content

[safe_memcpy_test] Explicit type for arguments of the vector constructor#16549

Merged
antoniovicente merged 2 commits intoenvoyproxy:mainfrom
rialg:issue-16542
May 19, 2021
Merged

[safe_memcpy_test] Explicit type for arguments of the vector constructor#16549
antoniovicente merged 2 commits intoenvoyproxy:mainfrom
rialg:issue-16542

Conversation

@rialg
Copy link
Copy Markdown
Contributor

@rialg rialg commented May 18, 2021

Signed-off-by: rialg grial@google.com

Commit Message: Explicit type for arguments of the vector constructor
Additional Description: Issue #16542
Risk Level: N/A

Signed-off-by: rialg <grial@google.com>
@rialg
Copy link
Copy Markdown
Contributor Author

rialg commented May 18, 2021

Please, @jmarantz and @antoniovicente , could you add yourselves as reviewers?

@jmarantz jmarantz self-assigned this May 18, 2021
Copy link
Copy Markdown
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.

thanks!

@jmarantz
Copy link
Copy Markdown
Contributor

/retest

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

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

see: more, trace.

@antoniovicente
Copy link
Copy Markdown
Contributor

Change looks good, but it would be good to hear from @Wyverald on wherever or not this fixes the issue he saw in the bazel build and how to repo that breakage.

@Wyverald
Copy link
Copy Markdown
Contributor

Thanks for the PR. Unfortunately there isn't an easy way to do a test CI run on a PR (unless we turn the pipeline on for all future Envoy PRs -- which we could certainly do). Is there a way to push this PR to a branch in the main Envoy repo (envoyproxy/envoy)? That would give us a way to test beforehand if this fixes it.

@rialg
Copy link
Copy Markdown
Contributor Author

rialg commented May 19, 2021

The presubmit pipeline failed due to low code coverage in the tests.

Code coverage for source/common/quic is lower than limit of 88.4 (88.3)

Should we lower this target to match the current value?

@jmarantz
Copy link
Copy Markdown
Contributor

I think once #16570 merges you should be good? @alyssawilk has another PR also which might just improve the coverage.

@jmarantz
Copy link
Copy Markdown
Contributor

go ahead an 'merge main' and this coverage issue should be resolved.

Signed-off-by: rialg <grial@google.com>
@antoniovicente
Copy link
Copy Markdown
Contributor

Thanks for the PR. Unfortunately there isn't an easy way to do a test CI run on a PR (unless we turn the pipeline on for all future Envoy PRs -- which we could certainly do). Is there a way to push this PR to a branch in the main Envoy repo (envoyproxy/envoy)? That would give us a way to test beforehand if this fixes it.

ok, we'll give it another shot if this change isn't sufficient.

@antoniovicente antoniovicente merged commit bf3e6a2 into envoyproxy:main May 19, 2021
@rialg rialg deleted the issue-16542 branch May 20, 2021 14:53
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants