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

feat: add ingressClassName value to ingress #527

Merged
merged 1 commit into from
Apr 1, 2022
Merged

feat: add ingressClassName value to ingress #527

merged 1 commit into from
Apr 1, 2022

Conversation

fkoetzner
Copy link
Contributor

What issues does your PR fix?

What does your PR do?

  • Adds the following values to allow setting ingressClassName on the Ingress resources:
    • ingress.web.ingressClassName
    • ingress.flower.ingressClassName

Checklist

For all Pull Requests

For releasing ONLY


P.S.: Sorry, I accidentally closed the original pull request #524.

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.

@fkoetzner this looks good now, with only one random diff to be fixed.

Also, you can re-open closed PRs, you don't have to make new ones, otherwise, it's sort of spammy.

thesuperzapper
thesuperzapper previously approved these changes Feb 23, 2022
@thesuperzapper
Copy link
Member

@fkoetzner I just realised that we haven't added ingressClassName to the v1beta1 versions of the Ingresses:

Can you clarify if v1beta1 ingresses have the ingressClassName field (as of Kubernetes 1.18+)?

  • If YES, then we need to also include the value in those files
  • If NO, then we should add a new validation to _helpers/validate-values.tpl that fails if ingress.apiVersion is networking.k8s.io/v1beta1 AND either ingressClassName is non-empty.

Signed-off-by: fkoetzner <florian.koetzner@baeckerai.de>
@fkoetzner
Copy link
Contributor Author

@fkoetzner I just realised that we haven't added ingressClassName to the v1beta1 versions of the Ingresses:

* [./templates/webserver/webserver-ingress-v1beta1.yaml](https://github.com/airflow-helm/charts/blob/main/charts/airflow/templates/webserver/webserver-ingress-v1beta1.yaml)

* [./templates/flower/flower-ingress-v1beta1.yaml](https://github.com/airflow-helm/charts/blob/main/charts/airflow/templates/flower/flower-ingress-v1beta1.yaml)

Can you clarify if v1beta1 ingresses have the ingressClassName field (as of Kubernetes 1.18+)?

* If YES, then we need to also include the value in those files

* If NO, then we should  add a new validation to [_helpers/validate-values.tpl](https://github.com/airflow-helm/charts/blob/df99a33e3278de9f6935e747e8317794966ed979/charts/airflow/templates/_helpers/validate-values.tpl#L94-L95) that fails if `ingress.apiVersion` is `networking.k8s.io/v1beta1` AND either `ingressClassName` is non-empty.

Yes, you're right. I thought it was only added to networking.k8s.io/v1, but I tested it with v1beta1 and it works. Also, this confirms that v1beta1 has this field.

@thesuperzapper thesuperzapper changed the title feat: add ingressClassName to web/flower ingress feat: add ingressClassName value to ingress Mar 22, 2022
@thesuperzapper thesuperzapper self-requested a review March 22, 2022 00:45
@thesuperzapper thesuperzapper added the status/ready-to-merge status - this will be merged into next release label Mar 22, 2022
@thesuperzapper thesuperzapper merged commit 205e988 into airflow-helm:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready-to-merge status - this will be merged into next release
Projects
Archived in project
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