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

Remove binding name #567

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Remove binding name #567

merged 6 commits into from
Jan 15, 2025

Conversation

masonj5n
Copy link
Member

@masonj5n masonj5n commented Jan 9, 2025

What

The API for kubernetes bindings is being updated and no longer includes the binding_name field. Instead, kubernetes endpoints will be scoped to the account instead of a specific operator.

How

Removes references to binding_name.

Breaking Changes

The ngrok API for kubernetes bindings itself is breaking with this change so this is just following that.
As far as the operator itself:

  • The KubernetesOperatorBinding resource had its Name field removed.
  • values.yaml has been updated to no longer include bindings.name

@github-actions github-actions bot added area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart labels Jan 9, 2025
@Alice-Lilith
Copy link
Contributor

🙏 looking forward to not getting my logs spammed with

2025-01-10T00:31:52Z	DEBUG	events	Failed to update KubernetesOperator/ngrok-operator: HTTP 422: The request parameter 'name' is unknown and not expected. The supported fields are: allowed_urls, csr, ingress_endpoint. Please check your API client implementation and review the API documentation: https://ngrok.com/docs/api#api-kubernetes-operators-update. [ERR_NGROK_214]

// +kubebuilder:validation:Required
// +kubebuilder:validation:Pattern=`^k8s[/][a-zA-Z0-9-]{1,63}$`
Name string `json:"name,omitempty"`

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we shouldn't leave this field in here but mark it as deprecated and change the validation to optional. The only thing I'm worried about if we remove it right now is if someone deploys this change and then rolls back to a previous version, what happens?

If there aren't a lot of users using bindings right now, I think its a lower risk change.

Copy link
Member Author

@masonj5n masonj5n Jan 10, 2025

Choose a reason for hiding this comment

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

Since we're breaking the API on the hosted side bindings won't work with this field anyway so I don't think it matters too much. Whenever we cut a release with these and a couple other upcoming changes that'll be the first release that works with bindings going forward

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@96ccc03). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../controller/ngrok/kubernetesoperator_controller.go 0.00% 4 Missing ⚠️
...ternal/controller/bindings/boundendpoint_poller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #567   +/-   ##
=======================================
  Coverage        ?   24.11%           
=======================================
  Files           ?       88           
  Lines           ?    12571           
  Branches        ?        0           
=======================================
  Hits            ?     3031           
  Misses          ?     9350           
  Partials        ?      190           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@masonj5n masonj5n marked this pull request as ready for review January 14, 2025 23:55
@masonj5n masonj5n requested a review from a team as a code owner January 14, 2025 23:55
@masonj5n masonj5n requested a review from jonstacks January 15, 2025 00:05
@masonj5n masonj5n added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 91d1ccb Jan 15, 2025
11 checks passed
@masonj5n masonj5n deleted the mason/remove_binding_name branch January 15, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants