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

printing name of the target when a policy fails to attach #3943

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

sadovnikov
Copy link
Contributor

What type of PR is this?

Policies in the v1.1.x versions will have multiple targets. This PR makes error messages on failures to attach to a target more specific.

Which issue(s) this PR fixes:

No issues were logged for this

@zirain, this PR replaces #3917, which I messed up with the wrong commits. It also includes changes in the test files.

@sadovnikov sadovnikov requested a review from a team as a code owner July 23, 2024 14:56
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.38%. Comparing base (a813084) to head (9424193).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3943      +/-   ##
==========================================
- Coverage   67.40%   67.38%   -0.02%     
==========================================
  Files         183      183              
  Lines       22435    22440       +5     
==========================================
- Hits        15123    15122       -1     
- Misses       6225     6227       +2     
- Partials     1087     1091       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sadovnikov
Copy link
Contributor Author

@zirain, I corrected the files with the expected test outcomes. Could you please approve the workflows?

@sadovnikov sadovnikov requested a review from zirain July 24, 2024 08:15
@sadovnikov
Copy link
Contributor Author

The trailing spaces in the test files are removed

Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for improving the status message, making it easier fo figure out which target is overriden.

@zhaohuabing zhaohuabing merged commit a0c0e48 into envoyproxy:main Jul 25, 2024
23 of 24 checks passed
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.

3 participants