Skip to content

Adds GatewayClass Status Support#150

Merged
danehans merged 1 commit intoenvoyproxy:mainfrom
danehans:gc_status_impl
Jul 18, 2022
Merged

Adds GatewayClass Status Support#150
danehans merged 1 commit intoenvoyproxy:mainfrom
danehans:gc_status_impl

Conversation

@danehans
Copy link
Copy Markdown
Contributor

@danehans danehans commented Jul 13, 2022

  • Adds Status pkg for managing status of supported resources.
  • Updates gatewayclass controller to set the status of managed gatewayclasses.

Fixes #147

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested a review from a team as a code owner July 13, 2022 20:04
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #150 (f481318) into main (ebca0a7) will decrease coverage by 1.58%.
The diff coverage is 47.59%.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
- Coverage   53.08%   51.50%   -1.59%     
==========================================
  Files          12       15       +3     
  Lines         486      666     +180     
==========================================
+ Hits          258      343      +85     
- Misses        210      299      +89     
- Partials       18       24       +6     
Impacted Files Coverage Δ
internal/status/gatewayclass.go 0.00% <0.00%> (ø)
internal/status/status.go 3.12% <3.12%> (ø)
pkg/provider/kubernetes/gatewayclass.go 63.96% <57.74%> (-8.38%) ⬇️
internal/status/conditions.go 93.87% <93.87%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebca0a7...f481318. Read the comment docs.

arkodg
arkodg previously approved these changes Jul 13, 2022
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

thanks for adding this, the status controller can be made a stand alone entity in a follow up PR using #147

@danehans danehans force-pushed the gc_status_impl branch 2 times, most recently from dba3598 to bfb5965 Compare July 14, 2022 02:10
@danehans danehans marked this pull request as draft July 14, 2022 04:59
Copy link
Copy Markdown
Contributor

@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.

This LGTM with one future request about ObservedGeneration - it's critical for Condition consumers to be able to be sure they're seeing the correct version of the status.

Comment thread internal/status/conditions.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably have a TODO here to pass the Generation as well, so we can set ObservedGeneration. Not blocking for this PR though.

Copy link
Copy Markdown
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Code LGTM. My one comment is that as we're reusing code from other projects (which I'm in favor of BTW), we should make sure we're doing some attribution/not falling afoul of licensing requirements by reusing that work. I'm not sure what the best approach to doing this is, since I expect we'll all be doing a fair amount of reuse -- perhaps we can add some documentation at the repo level indicating the projects that we're reusing code from and including whatever license info is required, rather than doing it on a file-by-file basis.

@danehans danehans force-pushed the gc_status_impl branch 2 times, most recently from a86d789 to f481318 Compare July 15, 2022 16:51
@danehans danehans marked this pull request as ready for review July 15, 2022 16:51
@danehans
Copy link
Copy Markdown
Contributor Author

@skriss @youngnick commit f481318 adds the Contour license header to src files lifted from Contour, PTAL.

@skriss
Copy link
Copy Markdown
Contributor

skriss commented Jul 15, 2022

@danehans my hope is that we can do something like #92 (comment) to avoid needing to do this per file, but until we resolve that, I think what you have works. The one other thing I would add is for any files that were not copied verbatim, adding some text indicating they've been modified from the original to satisfy https://github.com/envoyproxy/gateway/blob/main/LICENSE#L97-L98.

@danehans danehans force-pushed the gc_status_impl branch 3 times, most recently from 3dff7bc to a929339 Compare July 15, 2022 22:26
@danehans
Copy link
Copy Markdown
Contributor Author

@skriss @youngnick I've updated the relevant src files with a Contour attribution. I think this is acceptable until #92 (comment) is resolved, PTAL.

skriss
skriss previously approved these changes Jul 15, 2022
Copy link
Copy Markdown
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM pending a little merge conflict.

Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans danehans merged commit 8a2905e into envoyproxy:main Jul 18, 2022
@danehans danehans deleted the gc_status_impl branch July 18, 2022 22:10
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.

Design and Implement Initial Status Controller

5 participants