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: set Gateway Listener conditions #4186

Merged
merged 4 commits into from
Nov 30, 2021

Conversation

skriss
Copy link
Member

@skriss skriss commented Nov 17, 2021

Sets various Listener conditions according to
the Gateway API v1alpha2 spec. Also sets the
Gateway "Ready" condition to false if there
are invalid listeners.

Closes #3529.
Closes #4180.
Closes #4124.

Signed-off-by: Steve Kriss krisss@vmware.com

@skriss skriss requested a review from a team as a code owner November 17, 2021 15:48
@skriss skriss requested review from sunjayBhatia and youngnick and removed request for a team November 17, 2021 15:48
@skriss skriss added release-note/small A small change that needs one line of explanation in the release notes. release-note/minor A minor change that needs about a paragraph of explanation in the release notes. and removed release-note/small A small change that needs one line of explanation in the release notes. labels Nov 17, 2021
@skriss skriss requested a review from stevesloka November 17, 2021 15:52
@skriss
Copy link
Member Author

skriss commented Nov 17, 2021

Using the "hide whitespace" option on the diff will help a lot here.

I think there are probably still some additional situations for which we should be setting Listener conditions, but this set covered what seemed like the biggest ones / what we were already checking for in the processor logic. Any additional ones can be tacked on individually now that the scaffolding is there.

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #4186 (fe1863b) into main (9134b14) will increase coverage by 0.32%.
The diff coverage is 94.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4186      +/-   ##
==========================================
+ Coverage   76.09%   76.41%   +0.32%     
==========================================
  Files         111      111              
  Lines        9763     9883     +120     
==========================================
+ Hits         7429     7552     +123     
+ Misses       2168     2164       -4     
- Partials      166      167       +1     
Impacted Files Coverage Δ
internal/status/gatewaystatus.go 71.73% <86.20%> (+6.11%) ⬆️
internal/dag/gatewayapi_processor.go 96.66% <96.47%> (+2.11%) ⬆️

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

LGTM!

only addition would be to add a test somewhere to confirm that listener condition messages are appended if there are multiple issues, implementation looks good 👍🏽

@skriss
Copy link
Member Author

skriss commented Nov 17, 2021

LGTM!

only addition would be to add a test somewhere to confirm that listener condition messages are appended if there are multiple issues, implementation looks good 👍🏽

Good call, added a simple unit test for that method.

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, this looks great, thanks @skriss!

Sets various Listener conditions according to
the Gateway API v1alpha2 spec. Also sets the
Gateway "Ready" condition to false if there
are invalid listeners.

Closes projectcontour#3529.
Closes projectcontour#4180.
Updates projectcontour#4124.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss force-pushed the gw-listener-conditions branch from 52d27a3 to fe1863b Compare November 30, 2021 16:39
@skriss skriss merged commit 39a31e6 into projectcontour:main Nov 30, 2021
@skriss skriss deleted the gw-listener-conditions branch November 30, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
3 participants