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.

Sign up for GitHub

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

Jump to bottom

feat: expose envoyproxy image by proxyConfig #1203

Merged
merged 2 commits into from
Apr 13, 2023
Merged

feat: expose envoyproxy image by proxyConfig #1203

merged 2 commits into from
Apr 13, 2023

Conversation

Xunzhuo
Copy link
Member

@Xunzhuo Xunzhuo commented Mar 23, 2023

  1. add doc Customize EnvoyProxy
  2. add support to customize envoyproxy image

@Xunzhuo Xunzhuo requested a review from a team as a code owner March 23, 2023 08:01
@Xunzhuo Xunzhuo force-pushed the feat-expose-image branch from 43829c0 to f78bfca Compare March 23, 2023 08:07
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023 •
edited by codecov bot
Loading

Codecov Report

Merging #1203 (0ca2851) into main (5be0610) will decrease coverage by 0.10%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##             main    #1203      +/-   ##
==========================================
- Coverage   61.81%   61.71%   -0.10%     
==========================================
  Files          85       85              
  Lines       10749    10753       +4     
==========================================
- Hits         6644     6636       -8     
- Misses       3665     3675      +10     
- Partials      440      442       +2
Impacted Files Coverage Δ
api/config/v1alpha1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
internal/gatewayapi/listener.go 100.00% <ø> (ø)
internal/gatewayapi/translator.go 97.10% <ø> (ø)
internal/ir/infra.go 64.36% <ø> (+0.53%) ⬆️
api/config/v1alpha1/helpers.go 52.59% <66.66%> (+0.87%) ⬆️
...rnal/infrastructure/kubernetes/proxy_deployment.go 91.14% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@qicz
Copy link
Member

qicz commented Mar 23, 2023

The doc for custom envoy deployment resources is also provided. thanks

@arkodg
Copy link
Contributor

arkodg commented Mar 23, 2023

thanks @Xunzhuo, code looks good, added comments around clarity of the image compatibility and suggestions around docs (thanks for proactively adding them !).
This is needed to allow users to pull from private registries, but we must also ensure we can guarantee some API compatibility

@arkodg arkodg added this to the 0.4.0-rc.1 milestone Apr 6, 2023
@Xunzhuo Xunzhuo added release-blocker priority/high Label used to express the "high" priority level labels Apr 7, 2023
@Xunzhuo Xunzhuo self-assigned this Apr 7, 2023
@Xunzhuo Xunzhuo force-pushed the feat-expose-image branch 2 times, most recently from a2f0b1b to ecff567 Compare April 7, 2023 08:05
@arkodg
Copy link
Contributor

arkodg commented Apr 7, 2023

hey @Xunzhuo the API has changed since this PR started :)
the docs would need to be changed too, thanks in advance !

@qicz qicz mentioned this pull request Apr 11, 2023
Merged
@arkodg
Copy link
Contributor

arkodg commented Apr 11, 2023 •
edited
Loading

hey @Xunzhuo do you think this can get into 0.4.0-rc1 (releasing on April 13th) ?

This was referenced Apr 12, 2023
Closed
Closed
@Xunzhuo Xunzhuo force-pushed the feat-expose-image branch 8 times, most recently from bde3701 to cd1283e Compare April 12, 2023 11:57
@Xunzhuo
Copy link
Member Author

Xunzhuo commented Apr 12, 2023

@arkodg I have resolved all comments, plz take a look again! Thanks!

@Xunzhuo Xunzhuo force-pushed the feat-expose-image branch 2 times, most recently from 7407e3a to 1d51720 Compare April 12, 2023 12:01
@Xunzhuo Xunzhuo force-pushed the feat-expose-image branch from 1d51720 to 19326fe Compare April 12, 2023 12:09
@arkodg arkodg mentioned this pull request Apr 12, 2023
Closed
arkodg
arkodg approved these changes Apr 12, 2023
View reviewed changes
Copy link
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.

LGTM, thanks for also adding docs !
@zirain this PR might affect #1259 🙈

Alice-Lilith
Alice-Lilith approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

Thanks, looks good overall. I just found a couple of very minor nit-picks in the docs but let's not hold this PR up for those since we want to make sure this lands for 0.4.0-rc.1 tomorrow.

docs/latest/user/customize-envoyproxy.md
# Customize EnvoyProxy

Envoy Gateway provides a [EnvoyProxy][] CRD that can be linked to the ParametersRef
in GatewayClass y cluster admins to customize the managed EnvoyProxy Deployment and
Copy link
Member

Choose a reason for hiding this comment

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

nit: GatewayClass y cluster

docs/latest/user/customize-envoyproxy.md
EOF
```

After applying the config, you can get the envoyproxy pods, and see new annotations has been added.
Copy link
Member

Choose a reason for hiding this comment

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

nit: annotations has been -> annotations have been

@arkodg arkodg merged commit a5f3a1a into envoyproxy:main Apr 13, 2023
@zirain
Copy link
Member

zirain commented Apr 13, 2023

LGTM, thanks for also adding docs ! @zirain this PR might affect #1259 🙈

oh, no!

@qicz qicz mentioned this pull request Apr 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zirain zirain zirain left review comments

@qicz qicz qicz left review comments

@Alice-Lilith Alice-Lilith Alice-Lilith approved these changes

@arkodg arkodg arkodg approved these changes

Assignees

@Xunzhuo Xunzhuo

Labels
priority/high Label used to express the "high" priority level release-blocker
Projects
None yet
Milestone
0.4.0-rc.1
Development

Successfully merging this pull request may close these issues.
6 participants
@Xunzhuo @codecov-commenter @qicz @arkodg @zirain @Alice-Lilith