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

Replace principle by principal in kerberos-related code #43679

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

brouberol
Copy link
Contributor

Kerberos has the notion of a principal. "principle" seemed to be a typo.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

potiuk
potiuk previously requested changes Nov 5, 2024
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Even if it's a typo fix, it crosses airflow/providers boundaries - which means that you would have to implement compatibilty layer in both airflow and providers:

  1. so that new airflow supports old providers
  2. so that old airflow (2.8+ currently) supports new providers

I think the hassle is not worth the typo fix

@eladkal
Copy link
Contributor

eladkal commented Nov 5, 2024

I think the hassle is not worth the typo fix

We can just have a function with the correct name and deprecate the old one
given that we are going to remove all deprecations before 2.11 I think it's good chance to do it

Ah sorry this involves also Airflow core

Still I think we can do it with Airflow 3 and backport to 2.10.. this is pretty minor and easy to do.

@potiuk
Copy link
Member

potiuk commented Nov 5, 2024

Still I think we can do it with Airflow 3 and backport to 2.10.. this is pretty minor and easy to do.

If @brouberol wishes to do the back-compatiblity code for both issues, we can do it, yes.

@eladkal
Copy link
Contributor

eladkal commented Nov 6, 2024

OK lets take it by steps:

  1. changes to airflow/security/kerberos.py should be in a separated PR. Add new function get_kerberos_principal with the logic and deprecate get_kerberos_principle once this PR is merged to main branch, we can start similar PR for v2.10-test branch.
  2. once (1) is completed we can handle the provider changes

I can handle (1) if you prefer

@brouberol
Copy link
Contributor Author

I can give it a go, but I'll be a bit tied up in the coming days. Feel free to proceed with 1) if you want. If not, I'll try to address your comment when I have the time. Thanks again!

@eladkal
Copy link
Contributor

eladkal commented Nov 6, 2024

Cool but actually we need (2) ready before (1)

All you need to do here is to remove the changes to airflow/security/kerberos.py and tests/security/test_kerberos.py.

Keep the changes you have in providers but change the imports to:

#todo: remove try/exception when min airflow version is 3.0
try:
    from airflow.security.kerberos import get_kerberos_principal
except ModuleNotFoundError:
    from airflow.security.kerberos import get_kerberos_principle

Once you do that provider is compatible with both name and I can do the change to airflow core.

brouberol added a commit to brouberol/airflow that referenced this pull request Nov 18, 2024
This is related to apache#43679, in which the `airflow.security.kerberos.get_kerberos_principle` function was renamed `get_kerberos_principle`.

In this patch, we introduce a `try/except` block around this import,
getting ready to deprecate the typo-ed function in Airflow 3.0.

We also fix a innocuous typo in a unit test.

Signed-off-by: Balthazar Rouberol <[email protected]>
@brouberol
Copy link
Contributor Author

I've reworked this patch to only rename the affected function, and have opened #44150 that attempts to import the fixed function and falls back to the typoed one, as suggested in #43679 (comment)

@eladkal eladkal added area:core airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes and removed area:providers provider:apache-spark labels Nov 18, 2024
@eladkal eladkal added this to the Airflow 3.0.0 milestone Nov 18, 2024
@eladkal
Copy link
Contributor

eladkal commented Nov 18, 2024

Lets add 43679.misc.rst in newsfragments.
Just need to say in the file that we renamed the function due to misspelling. This will be used to generate the release notes.

@brouberol brouberol force-pushed the fix-kerberos-typos branch 2 times, most recently from 8d3c663 to d30cc35 Compare November 19, 2024 09:22
eladkal added a commit that referenced this pull request Nov 19, 2024
* spark-submit: replace `principle` by `principal`

This is related to #43679, in which the `airflow.security.kerberos.get_kerberos_principle` function was renamed `get_kerberos_principle`.

In this patch, we introduce a `try/except` block around this import,
getting ready to deprecate the typo-ed function in Airflow 3.0.

We also fix a innocuous typo in a unit test.

Signed-off-by: Balthazar Rouberol <[email protected]>

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

---------

Signed-off-by: Balthazar Rouberol <[email protected]>
Co-authored-by: Elad Kalif <[email protected]>
Co-authored-by: rom sharon <[email protected]>
@eladkal eladkal merged commit 8daf74e into apache:main Nov 19, 2024
45 checks passed
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* spark-submit: replace `principle` by `principal`

This is related to apache#43679, in which the `airflow.security.kerberos.get_kerberos_principle` function was renamed `get_kerberos_principle`.

In this patch, we introduce a `try/except` block around this import,
getting ready to deprecate the typo-ed function in Airflow 3.0.

We also fix a innocuous typo in a unit test.

Signed-off-by: Balthazar Rouberol <[email protected]>

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

* Update providers/src/airflow/providers/apache/spark/hooks/spark_submit.py

---------

Signed-off-by: Balthazar Rouberol <[email protected]>
Co-authored-by: Elad Kalif <[email protected]>
Co-authored-by: rom sharon <[email protected]>
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes area:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants