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

Make sure impersonation tests use a temporary AIRFLOW_HOME #33422

Conversation

Taragolis
Copy link
Contributor

Continuation of #30313

The impersonation tests of ours modify permissions for the AIRFLOW_HOME recursively. This is quite a bad idea when the tests are executed inside breeze with mapped folders (logs) because the logs folder ends up with being not readable by the owner.

Changing AIRFLOW_HOME in the fixture performig permission change should help with that.

^ 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.

@Taragolis Taragolis force-pushed the do-not-operate-on-airflow-home-for-impersonation-2 branch from 3079fe6 to 6b4902b Compare August 15, 2023 20:41
@eladkal
Copy link
Contributor

eladkal commented Aug 15, 2023

Its great to see you again @Taragolis !

@Taragolis
Copy link
Contributor Author

Well, sqlite test failure it is a bit expected, because impersonate user doesn't have access to /root/airflow/sqlite/airflow.db 🤔

@potiuk
Copy link
Member

potiuk commented Aug 16, 2023

Well, sqlite test failure it is a bit expected, because impersonate user doesn't have access to /root/airflow/sqlite/airflow.db 🤔

Should not it be using the database in the temp AIRFLOW_HOME Then? We could simply add write access to that sqlite instanace and simply initialise and use it before every impersonation test as part of the "create_airflow_home" fixture. that sounds like best approach ?

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.

As @Taragolis noted - the sqlite tests need a bit tweaking :)

@Taragolis
Copy link
Contributor Author

Should not it be using the database in the temp AIRFLOW_HOME Then?

Unfortunetly we initialise SQLite DB in {AIRFLOW_HOME}/sqlite/airflow.db, I've tried re-initialise it in test runtime in another place but without success (yet??).

The easiest solution in case of sqlite backend in breeze it would create DB in directory with 777 permissions and outside of AIRFLOW_HOME so everyone in breeze have access to this sqlite on read and write, I guess it is the same requirement for someone who want to use impersonation + sqlite

@potiuk
Copy link
Member

potiuk commented Aug 16, 2023

Feel free to merge as-is if you are ok with it too and do not want to make any more changes.

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

This is nice, Looks Good +1

@Taragolis Taragolis merged commit e90febc into apache:main Aug 16, 2023
42 checks passed
@Taragolis Taragolis deleted the do-not-operate-on-airflow-home-for-impersonation-2 branch August 16, 2023 23:22
ferruzzi pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants