Skip to content

Use new types to process resource changes #124

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

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

pleshakov
Copy link
Contributor

Proposed changes

This PR plugs in the new types introduced in #121 to process
changes to resources and generate NGINX configuration.

The installation manifests and examples were also updated to support the Gateway resource.

Note: the status updating was removed. In will be reintroduced in the next PR -- an updated version what works with the new types.

@pleshakov pleshakov added the enhancement New feature or request label Jun 6, 2022
@pleshakov pleshakov requested a review from kate-osborn June 6, 2022 15:34
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor comments/questions

if gw.Name == impl.ControllerName() {
impl.Logger().Info("Found correct Gateway resource",
if gw.Namespace != impl.conf.GatewayNsName.Namespace || gw.Name != impl.conf.GatewayNsName.Name {
impl.Logger().Info("Gateway was upserted but ignored based on its namespace/name",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add the expected ns name to this log so the user has the ability to fix the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this always will be an error though. Because let's say the admin deploys multiple NGINX Gateways, each watching its own Gateway resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. Once we support configurable gateway names we can change this back.

@pleshakov pleshakov force-pushed the feature/listeners-config-gen branch from a3df36f to 107c3e3 Compare June 9, 2022 19:20
@pleshakov pleshakov requested a review from kate-osborn June 9, 2022 19:27
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

🚀

pleshakov added 2 commits June 9, 2022 13:53
This commit plugs in the new types introduced in 0a39e3f to process
changes to resources and generate NGINX configuration.
@pleshakov pleshakov force-pushed the feature/listeners-config-gen branch from 107c3e3 to 6ba113d Compare June 9, 2022 19:54
@pleshakov pleshakov merged commit 0e9ce25 into feature/listeners Jun 9, 2022
@pleshakov pleshakov deleted the feature/listeners-config-gen branch July 5, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants