Skip to content

feat: allow users to set ingressClassName #426

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
Closed
loomsen wants to merge 1 commit into from
Closed

feat: allow users to set ingressClassName #426

loomsen wants to merge 1 commit into from

Conversation

@loomsen
Copy link

@loomsen loomsen commented Sep 10, 2021 •
edited by thesuperzapper
Loading

Signed-off-by: Norbert Varzariu [email protected]

What issues does your PR fix?

What does your PR do?

This PR adds the ingressClassName attribute to ingress objects.

Checklist

For all Pull Requests

@loomsen loomsen mentioned this pull request Sep 13, 2021
Closed
@LvffY LvffY mentioned this pull request Oct 17, 2021
Open
@thesuperzapper
Copy link
Member

thesuperzapper commented Oct 25, 2021

@loomsen sorry for the late reply, thanks for the PR!

It needs a few changes so we can merge:

  1. Don't create the CHANGELOG or bump the Chart.yaml version (we do this when we release, not on every commit)
  2. Try not to cause diffs outside your intended changes (including simple stuff like removing spaces), it makes it hard to review
  3. Rather than adding a value ingress.ingressClassName (which sets ingressClassName for all Ingress), we should create one for each flower/web ingress: (see my proposal: allow users to set ingressClassName #427)

PS: if you arent wanting to move forwards with it, I am happy to make a separate PR.

thesuperzapper
thesuperzapper requested changes Dec 14, 2021
View reviewed changes
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@loomsen do you have time to address the comments I made in #426 (comment)?

@thesuperzapper thesuperzapper changed the title add ingress class name to ingress definitions, it is required since 1.19 feat: allow users to set ingressClassName Dec 14, 2021
@loomsen
Copy link
Author

loomsen commented Dec 15, 2021

Hi @thesuperzapper ,

Sorry, I've been working for a different customer on another project, so I probably won't have the time to fix it. Please go ahead and thank you very much 😊

@stale stale bot added the lifecycle/stale lifecycle - this is stale label Feb 13, 2022
@thesuperzapper thesuperzapper mentioned this pull request Feb 17, 2022
Closed
@fkoetzner fkoetzner mentioned this pull request Feb 18, 2022
Closed
6 tasks
@stale stale bot closed this Feb 20, 2022
@thesuperzapper
Copy link
Member

thesuperzapper commented Feb 22, 2022

Work is continuing in PR #524

@fkoetzner fkoetzner mentioned this pull request Feb 22, 2022
Merged
6 tasks
@thesuperzapper thesuperzapper added status/duplicate status - this issue or pull request already exists and removed lifecycle/stale lifecycle - this is stale labels Apr 3, 2022
@airflow-helm airflow-helm deleted a comment from stale bot Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thesuperzapper thesuperzapper thesuperzapper requested changes

@gsemet gsemet Awaiting requested review from gsemet

Assignees

No one assigned

Labels

status/duplicate status - this issue or pull request already exists

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

allow users to set ingressClassName Ingress Class Name reqired since k8s 1.19

2 participants

@loomsen @thesuperzapper