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

internal/dag: Loosen restrictions on duplicate include detection #5017

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

sunjayBhatia
Copy link
Member

Empty or lone prefix path matches on "/" are now not considered duplicates, due to complications with existing examples and possible wide range of users relying on the previous less restrictive behavior.

Updates release note on duplicate include detection to major and adds some more detail

Empty or lone prefix path matches on "/" are now not considered
duplicates, due to complications with existing examples and possible
wide range of users relying on the previous less restrictive behavior.

Updates release note on duplicate include detection to major and adds
some more detail

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Jan 27, 2023
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner January 27, 2023 20:04
@sunjayBhatia sunjayBhatia requested review from stevesloka and skriss and removed request for a team January 27, 2023 20:04
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #5017 (6bf0905) into main (c12811f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5017      +/-   ##
==========================================
+ Coverage   77.47%   77.49%   +0.01%     
==========================================
  Files         138      138              
  Lines       16894    16904      +10     
==========================================
+ Hits        13088    13099      +11     
+ Misses       3551     3550       -1     
  Partials      255      255              
Impacted Files Coverage Δ
internal/dag/httpproxy_processor.go 92.50% <100.00%> (+0.15%) ⬆️

@skriss
Copy link
Member

skriss commented Jan 27, 2023

Yep, this looks sensible for now and we'll revisit more broadly with 1.25.

(As we discussed, it's unlikely that there is much usage of duplicate non-default include conditions, because if they were adjacent in the list of includes they already would've been flagged as duplicates. They would've had to be separated by a non-duplicate include in the middle in order to get through. Just doesn't seem very likely. That's why we're not too concerned about behavior for that case changing)

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia requested a review from skriss January 27, 2023 20:31
@sunjayBhatia sunjayBhatia added this to the 1.24.0 milestone Jan 27, 2023
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@skriss skriss merged commit 842361e into projectcontour:main Jan 30, 2023
@sunjayBhatia sunjayBhatia deleted the loosen-duplicate-include branch January 30, 2023 18:24
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…jectcontour#5017)

Empty or lone prefix path matches on "/" are now not considered
duplicates, due to complications with existing examples and possible
wide range of users relying on the previous less restrictive behavior.

Updates release note on duplicate include detection to major and adds
some more detail

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…jectcontour#5017)

Empty or lone prefix path matches on "/" are now not considered
duplicates, due to complications with existing examples and possible
wide range of users relying on the previous less restrictive behavior.

Updates release note on duplicate include detection to major and adds
some more detail

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
vmw-yingy pushed a commit to vmw-yingy/contour that referenced this pull request Feb 28, 2023
…jectcontour#5017)

Empty or lone prefix path matches on "/" are now not considered
duplicates, due to complications with existing examples and possible
wide range of users relying on the previous less restrictive behavior.

Updates release note on duplicate include detection to major and adds
some more detail

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants