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(dashboard): Only set ingressClass annotation when kubernetesCRD provider is listening for it. #1098

Closed
wants to merge 1 commit into from

Conversation

oscrx
Copy link

@oscrx oscrx commented Jun 23, 2024

What does this PR do?

fixes #1096

Motivation

The included dashboard ingressRoute is broken when the release name is not Traefik.

This is one of the possible fixes for it. But honestly it made me doubt if the implementation of custom ingressClass names are implemented correctly in this chart...

I split the changes into 2 commits to make reviewing easier.

More

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

@samox73
Copy link
Contributor

samox73 commented Jun 24, 2024

please update your PR to not reformat the test files

@mloiseleur
Copy link
Contributor

@oscrx Thanks for this PR. It seems a good way to fix it.

Would you please also update healthcheck-ingressroute.yaml template ?

Wdyt about adding a complete example on IngressClass with this Chart in EXAMPLES.md ?

@oscrx
Copy link
Author

oscrx commented Jun 24, 2024

please update your PR to not reformat the test files

I split it up in a separate commit to be ahead of your comment. I have no strong feelings about it but I like consistency. All the other test files had indent's but that's something for later I guess.

@oscrx Thanks for this PR. It seems a good way to fix it.

Would you please also update healthcheck-ingressroute.yaml template ?

Wdyt about adding a complete example on IngressClass with this Chart in EXAMPLES.md ?

I'll have to look into in after work. Sounds like a good idea 👍🏻

@mloiseleur
Copy link
Contributor

I split it up in a separate commit to be ahead of your comment. I have no strong feelings about it but I like consistency. All the other test files had indent's but that's something for later I guess.

Feel free to do it in an other PR. We can even add a linter for this.

…rovider is listening for it.

Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
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
Copy link
Contributor

@oscrx We are only missing the EXAMPLES.md and we are good to go.

@joaolongo
Copy link

Is this stale or it is going to be included on 28.4.0?

@oscrx
Copy link
Author

oscrx commented Jul 1, 2024

I don't have time currently to work on this/the example. Is it required or can it be another pr? If it's required it'll have to wait until next week.

@mloiseleur
Copy link
Contributor

Since we want to include this fix in next release, and since I was not able to commit/push on your branch, I've updated this PR with an example in #1109.

@darkweaver87
Copy link
Contributor

darkweaver87 commented Jul 2, 2024

Closing in favor of #1109. Thanks for your contribution @oscrx 🥇

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.

Dashboard starts returning 404 after 28.2.0 update
5 participants