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

fix: change annotation LBSvcRedirectHTTP to "https" #750

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a-mader
Copy link

@a-mader a-mader commented Sep 16, 2024

Hi,

I think this should be renamed from "http-to-http" to "http-to-https" and this would be a breaking change.

Closes #656

Kind regards

@a-mader a-mader requested a review from a team as a code owner September 16, 2024 14:31
@apricote
Copy link
Member

Thanks for the PR @a-mader,

this is indeed a breaking change, so I will not merge it as is.

I wrote a bit about the origin of the current annotation and the alternative I would like to see in #656 (comment).

If you want you can adjust this PR to match my suggestion in the linked comment.

@apricote apricote self-requested a review September 17, 2024 06:22
@apricote apricote added the enhancement New feature or request label Sep 17, 2024
@a-mader
Copy link
Author

a-mader commented Sep 20, 2024

Hi @apricote,

what do you think about having both annotations available, but marking the old one for deprecation?

e.g.

  • LBSvcRedirectHTTP marked for deprecation
  • LBSvcRedirectHTTPS new annotation
// MARKED FOR DEPRECATION (LBSvcRedirectHTTP create a redirect from HTTP to HTTPS. HTTPS only.)
LBSvcRedirectHTTP Name = "load-balancer.hetzner.cloud/http-redirect-http"

// LBSvcRedirectHTTP create a redirect from HTTP to HTTPS. HTTPS only. Bool either "true" or "false"
LBSvcRedirectHTTPS Name = "load-balancer.hetzner.cloud/http-redirect-https"

and update the documentation to use the new annotation.

Kind regards

@apricote
Copy link
Member

I would prefer to not mark LBSvcRedirectHTTP as deprecated. After all it is the one matching the API.

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.

Inconsistency in the naming for load balancer annotation.
2 participants