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(Traefik Proxy): allowEmptyServices not disabled when set to false #1256

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MaximilianMeister
Copy link

@MaximilianMeister MaximilianMeister commented Nov 16, 2024

What does this PR do?

Explicitly disallow empty services when values are set this way:

providers:
  kubernetesCRD:
    allowEmptyServices: false
  kubernetesIngress:
    allowEmptyServices: false

Motivation

The bug is if we don't explicitly set the cmdline flags to false, they will always default to true:

More

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

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.

Thanks for your PR. Could you add the corresponding unit tests ?

@MaximilianMeister
Copy link
Author

Thanks for your PR. Could you add the corresponding unit tests ?

@darkweaver87 added

@mloiseleur mloiseleur changed the title Explicitly disallow empty services fix(Traefik Proxy): allowEmptyServices not disabled when set to false Nov 18, 2024
@MaximilianMeister MaximilianMeister force-pushed the disallow-empty-services branch 2 times, most recently from 05b0c54 to 1a480bd Compare November 18, 2024 07:52
@mloiseleur
Copy link
Contributor

@MaximilianMeister I updated this PR title following conventional commit.

Feel free to update my edit if I missed something.

@MaximilianMeister
Copy link
Author

MaximilianMeister commented Nov 18, 2024

I updated this PR title following conventional commit.

thanks, i'll update the commit messages accordingly

If we don't explicitly set the cmdline flags to false, they will default
to true
@mloiseleur
Copy link
Contributor

There is a tiny indentation space missed. Otherwise, LGTM. Thanks 👍

@MaximilianMeister
Copy link
Author

There is a tiny indentation space missed. Otherwise, LGTM. Thanks 👍

thanks, i rebased with your suggestion, please have another look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants