potiuk commented on a change in pull request #4938: [AIRFLOW-4117]
Multi-staging Image - Travis CI tests [Step 3/3]
URL: https://github.com/apache/airflow/pull/4938#discussion_r272791314
##########
File path: tests/test_impersonation.py
##########
@@ -68,9 +83,11 @@ def setUp(self):
"current user have permission to run 'useradd' without a
password "
"prompt (check sudoers file)?"
)
+ self.update_database_permissions(revoke=False)
def tearDown(self):
subprocess.check_output(['sudo', 'userdel', '-r', TEST_USER])
+ self.update_database_permissions(revoke=True)
Review comment:
I think it is definitely woth it.
I think setting the permissions should only be done for the time the
impersonation tests are run. Setting it always - even in case of postgres/mysql
tests - is kind of excessive and I don't like to add more permissions than
needed (and for longer than needed) in general.
Another reason is that we might hide some problem if the permissions are set
for all tests. The other tests should work regardless if the group/other
permissions are set. Of course likelihood of it is very low, but still those
are not "typical" conditions to have group/other access to sqlite database.
It's quite rare case.
Yet another reason (and the most important) is that it was somehow magical
before. I only knew about the permissions in CI scripts because I was deeply
looking into them and (luckily) there was the comment that those lines are
needed for impersonation tests. But it was really not related to the tests
other than comment mentioned impersonation. Most people won't even look at CI
scripts and if something gets wrong they would be surprised how it works at
all. Having the explicit code to set it in the setup/teardown is much cleaner.
IMHO it falls into the "explicit is better than implicit" zen :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
