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

fix: 💥 BREAKING CHANGE on healthchecks and traefik port #898

Merged
merged 3 commits into from
Aug 1, 2023
Merged

fix: 💥 BREAKING CHANGE on healthchecks and traefik port #898

merged 3 commits into from
Aug 1, 2023

Conversation

mloiseleur
Copy link
Contributor

@mloiseleur mloiseleur commented Jul 28, 2023 •
edited by darkweaver87
Loading

What does this PR do?

  1. It move optional healthchecksPort and healthchecksScheme to deployment section.
  2. It ensures that traefik, web or websecure ports can be nullified

Motivation

  1. From a Kubernetes perspective, they are in fact linked to deployment, not a port
  2. Fix It's impossible to disable a port defined in the default config by setting it to null #890 in a more robust way

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@mloiseleur mloiseleur added kind/bug/fix kind/breaking This PR introduce a breaking change labels Jul 28, 2023
@mloiseleur mloiseleur mentioned this pull request Jul 28, 2023
Closed
2 tasks
@mloiseleur mloiseleur changed the title fix: 🐛 💥 BREAKING CHANGE on healthchecks and traefik port fix: 💥 BREAKING CHANGE on healthchecks and traefik port Jul 28, 2023
mantoine96
mantoine96 approved these changes Jul 28, 2023
View reviewed changes
Copy link

@mantoine96 mantoine96 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 the PR. I think the change looks a lot better now and feels like a robust solution. I've left 2 minor nits on typos, but otherwise this looks exactly like what we need to address #890 once and for all

traefik/templates/_podtemplate.tpl Outdated Show resolved Hide resolved
traefik/tests/ports-config_test.yaml Outdated Show resolved Hide resolved
darkweaver87
darkweaver87 approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

mloiseleur and others added 3 commits August 1, 2023 06:46
@mloiseleur @traefiker
fix: 🐛 💥 BREAKING CHANGE on healthchecks and traefik port
276edbf
@mloiseleur @mantoine96 @traefiker
Update traefik/templates/_podtemplate.tpl
42548d0 
Co-authored-by: Matthieu ANTOINE <matthieu@matthieu-antoine.me>
@mloiseleur @mantoine96 @traefiker
Update traefik/tests/ports-config_test.yaml
35c27b5 
Co-authored-by: Matthieu ANTOINE <matthieu@matthieu-antoine.me>
@traefiker traefiker merged commit d335f72 into traefik:master Aug 1, 2023
@mloiseleur mloiseleur deleted the fix/port_disabling branch August 1, 2023 07:05
@mloiseleur mloiseleur mentioned this pull request Aug 10, 2023
Merged
@renovate renovate bot mentioned this pull request Sep 8, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantoine96 mantoine96 mantoine96 approved these changes

@darkweaver87 darkweaver87 darkweaver87 approved these changes

Assignees
No one assigned
Labels
kind/breaking This PR introduce a breaking change kind/bug/fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It's impossible to disable a port defined in the default config by setting it to null
4 participants
@mloiseleur @darkweaver87 @mantoine96 @traefiker