Support .airflowignore for plugins by j-y-matsubara · Pull Request #9531 · apache/airflow

j-y-matsubara · 2020-06-26T10:56:50Z

This PR is to support .airflowinignore in the PLUGIN FOLDER.
Specifies intentionally files / directories in the PlUGINS_FOLDER to ignore by Airflow.

Issues Link:
#9413

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

mik-laj · 2020-06-26T14:14:43Z

airflow/plugins_manager.py

Hello.
This code looks very similar to:
https://github.com/apache/airflow/blob/master/airflow/utils/file.py#L125
I think you can make a generator that will take the starting path and yield for each file to load.
Best regards,
Kamil

You are right.
In fact, this is pretty much the same code.

I made a generator. What do you think of this?
（Perhaps we can make the generator func share with file.py but....）

I like this code now. :-)

Moving this method to airflow.utils.file is a good idea. This will allow us to delete the duplicate code.

mik-laj · 2020-06-28T06:45:06Z

docs/concepts.rst

Why the new file name? In my opinion, you can use .airflowignore and it will be easier to use.

I agree with your opinion.

In my opinion, you can use .airflowignore and it will be easier to use.

It was fixed from .pluginignore to .airflowignore . :-)

mik-laj · 2020-06-28T06:50:47Z

Could you add some tests? For plugin managers testing, tests.test_utils.mock_plugins.mock_plugin_manager may be helpful. A perfect test can create the required files in a temporary directory (NamedTemporaryFile) and then check if the plugins have been loaded.

j-y-matsubara · 2020-06-28T15:36:07Z

Could you add some tests? For plugin managers testing, tests.test_utils.mock_plugins.mock_plugin_manager may be helpful. A perfect test can create the required files in a temporary directory (NamedTemporaryFile) and then check if the plugins have been loaded.

Thank you for your advice.
I added a test.
And moved method to airflow.utils.file.

j-y-matsubara · 2020-06-30T09:30:55Z

I think the one failure of tests doesn't seem to have anything to do with this PR..... (##[error]Process completed with exit code 137. Probably not enough memory. )
The same error occurs in other PRs.

mik-laj · 2020-07-01T12:46:06Z

@turbaszek Can I ask for review?

turbaszek · 2020-07-01T13:21:49Z

airflow/plugins_manager.py

Should we make it a constant?

It's not particularly necessary.
I fixed!

turbaszek · 2020-07-01T13:22:45Z

airflow/plugins_manager.py

Is this cast to str necessary?

It is not necessary.
I fixed.

turbaszek · 2020-07-01T13:23:47Z

airflow/plugins_manager.py

Does this part have to be in try clause?

No....
I fixed

turbaszek · 2020-07-01T13:25:41Z

airflow/plugins_manager.py

Suggested change

for mod_attr_value in list(mod.__dict__.values()):

if is_valid_plugin(mod_attr_value):

plugin_instance = mod_attr_value()

plugins.append(plugin_instance)

for mod_attr_value in (m for m in mod.__dict__.values() if is_valid_plugin(m)):

plugin_instance = mod_attr_value()

plugins.append(plugin_instance)

No strong opinion here but in this way we may be able to fix the # pylint: disable=too-many-nested-blocks

Beautiful code!
I reflect this.

turbaszek · 2020-07-01T13:30:43Z

airflow/utils/file.py

Should we compile this only once?

It is not necessary.
I fixed.

turbaszek · 2020-07-01T13:34:05Z

airflow/utils/file.py

Suggested change

for subdir in dirs:

patterns_by_dir[os.path.join(root, subdir)] = patterns.copy()

patterns_by_dir = {os.path.join(root, sd): patterns.copy() for sd in dirs}

WDYT? Also, do we have to create copy of patterns each time?

This is necessary.
A canonical pattern that is evaluated in a parent directory must also be evaluated in its parent's child directories.　At least that's how .airflowignore (selection of dag) is currently specificated.

turbaszek · 2020-07-01T13:35:05Z

airflow/utils/file.py

Suggested change

if any([re.findall(p, file_path) for p in patterns]):

if any(re.findall(p, file_path) for p in patterns):

Should work also

Yes.
I fixed.

j-y-matsubara · 2020-07-01T16:11:30Z

@turbaszek
Thank you very much for the review.
I have corrected the code you pointed out!

Would this be okay?

j-y-matsubara · 2020-07-01T16:55:08Z

There was an error.
I will fix it.

j-y-matsubara · 2020-07-02T09:30:23Z

.... I think the two failure of k8s tests doesn't seem to have anything to do with this PR. The same error occurs in other PRs. :(

mik-laj · 2020-07-02T09:35:39Z

Please ignore it. It is not related.

turbaszek · 2020-07-02T10:28:02Z

airflow/utils/file.py

Suggested change

ignore_list_file: str) -> Generator[str, None, None]:

"""

Search the file and return the path of the file that should not be ignored.

:param base_dir_path: the base path to be searched for.

:param ignore_file_list_name: the file name in which specifies a regular expression pattern is written.

ignore_file_name: str) -> Generator[str, None, None]:

"""

Search the file and return the path of the file that should not be ignored.

:param base_dir_path: the base path to be searched for.

:param ignore_file_name: the file name in which specifies a regular expression pattern is written.

WDYT?

I'm sorry.
It's my simple mistake.
I fixed.

j-y-matsubara · 2020-07-03T06:11:47Z

The same k8s error that has nothing to do with this PR as last time.

turbaszek · 2020-07-03T07:16:42Z

tests/plugins/test_plugin_ignore.py

Can you please use the ctx manager?

with open(...) as file: file.write()

Of course!
I fixed.

turbaszek · 2020-07-03T07:19:58Z

tests/plugins/test_plugin_ignore.py

Suggested change

for path in detected_files:

self.assertNotIn(path, should_ignore_files)

self.assertEqual(detected_files & should_ignore_files, set())

WDYT?

It's efficient!
I fixed!

turbaszek · 2020-07-03T07:21:58Z

tests/plugins/test_ignore/subdir1/test_load_sub1.py

What is the purpose of those files? I think I don't see in tests

These files were used to files that should not be ignored.
( self.assertEqual(detected_files, should_not_ignore_files) Line 87 (now 95)of the test_plugin_ignore.py. )

But....
I fixed these files and ".airflowignore" files to be generated by test_plugin_ignore.py, and delete pull files!

turbaszek · 2020-07-03T15:25:39Z

tests/plugins/test_plugin_ignore.py

Is content of those files important? There's a lot of repeated code so I would opt for some loop like:

for file_path, content in files_content: with open(file_path) as f: f.wrtie(content)

Do you think it will make the code clearer?

Yes!
The notation you suggest is better than the existing my code.

I fixed.

turbaszek · 2020-07-03T20:58:42Z

@j-y-matsubara would you mind a rebase? The k8s tests should be fixed now

j-y-matsubara · 2020-07-04T03:28:54Z

@j-y-matsubara would you mind a rebase? The k8s tests should be fixed now

Sure.

All tests were passed!

ldacey · 2021-12-03T17:12:41Z

Is it better to use a .airflowignore in the dags folder and the plugins folder?

My plugins folder is like this:

/plugins
  - /operators
  - /hooks
  - /sensors
  - /sql
  - /tests

Airflow needs access to the hooks and operators (my DAGs import them). It does not need access to the tests folder, and maybe not the SQL folder (some DAGs/operators read the SQL scripts from this path).

Support .airflowignore for plugins : .pluginingore

673d3fb

boring-cyborg bot added area:docs area:plugins labels Jun 26, 2020

j-y-matsubara force-pushed the develop_plugin branch from ec2b43e to bb4568c Compare June 26, 2020 11:08

j-y-matsubara added 2 commits June 26, 2020 20:13

Fixed doc

2e87142

Fixed closed apache#9413

8bbe464

j-y-matsubara force-pushed the develop_plugin branch from bb4568c to 8bbe464 Compare June 26, 2020 11:13

mik-laj reviewed Jun 26, 2020

View reviewed changes

Make a generator

e7a7676

j-y-matsubara requested a review from mik-laj June 27, 2020 10:02

Fixed static check

b09bdf3

mik-laj reviewed Jun 28, 2020

View reviewed changes

j-y-matsubara added 2 commits June 29, 2020 00:14

Add test and moving method to airflow.utils.file

85502d7

Fixed

912f59e

j-y-matsubara added 2 commits June 29, 2020 00:39

Fixed static check

252adae

Fixed

855d8bc

j-y-matsubara changed the title ~~Support .airflowignore for plugins : .pluginingore~~ Support .airflowignore for plugins Jun 28, 2020

j-y-matsubara requested a review from mik-laj June 29, 2020 01:03

j-y-matsubara force-pushed the develop_plugin branch from 98631de to 16173d0 Compare June 30, 2020 08:04

Fixed Error

df7b2b1

j-y-matsubara force-pushed the develop_plugin branch from 16173d0 to 96f822f Compare June 30, 2020 08:36

Retest

42dc085

j-y-matsubara force-pushed the develop_plugin branch from 565c3c0 to 42dc085 Compare July 1, 2020 11:19

turbaszek reviewed Jul 1, 2020

View reviewed changes

Fixed

734f47d

j-y-matsubara requested a review from turbaszek July 1, 2020 16:15

Fixed

cef299b

j-y-matsubara added 2 commits July 2, 2020 12:04

Fixed

8ff278b

Retest

813b1aa

j-y-matsubara force-pushed the develop_plugin branch from ff074ce to 813b1aa Compare July 2, 2020 03:04

turbaszek reviewed Jul 2, 2020

View reviewed changes

turbaszek approved these changes Jul 2, 2020

View reviewed changes

turbaszek approved these changes Jul 3, 2020

View reviewed changes

turbaszek reviewed Jul 3, 2020

View reviewed changes

turbaszek approved these changes Jul 3, 2020

View reviewed changes

j-y-matsubara added 3 commits July 4, 2020 11:28

Fixed param name

ba15144

Fixed

a97b271

Fixed code

307aa4d

j-y-matsubara force-pushed the develop_plugin branch from 9fad794 to 307aa4d Compare July 4, 2020 02:29

turbaszek approved these changes Jul 4, 2020

View reviewed changes

turbaszek merged commit 5670e6f into apache:master Jul 4, 2020

turbaszek mentioned this pull request Jul 6, 2020

Support airflowignore for plugins #9413

Closed

-            for mod_attr_value in list(mod.__dict__.values()):
-                if is_valid_plugin(mod_attr_value):
-                    plugin_instance = mod_attr_value()
-                    plugins.append(plugin_instance)
+            for mod_attr_value in (m for m in mod.__dict__.values() if is_valid_plugin(m)):
+                plugin_instance = mod_attr_value()
+                plugins.append(plugin_instance)

-        for subdir in dirs:
-            patterns_by_dir[os.path.join(root, subdir)] = patterns.copy()
+        patterns_by_dir  = {os.path.join(root, sd): patterns.copy() for sd in dirs}

	if any([re.findall(p, file_path) for p in patterns]):
	if any(re.findall(p, file_path) for p in patterns):

-        ignore_list_file: str) -> Generator[str, None, None]:
-    """
-    Search the file and return the path of the file that should not be ignored.
-    :param base_dir_path: the base path to be searched for.
-    :param ignore_file_list_name: the file name in which specifies a regular expression pattern is written.
+        ignore_file_name: str) -> Generator[str, None, None]:
+    """
+    Search the file and return the path of the file that should not be ignored.
+    :param base_dir_path: the base path to be searched for.
+    :param ignore_file_name: the file name in which specifies a regular expression pattern is written.

	for path in detected_files:
	self.assertNotIn(path, should_ignore_files)
	self.assertEqual(detected_files & should_ignore_files, set())

Conversation

j-y-matsubara commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj commented Jun 28, 2020

Uh oh!

j-y-matsubara commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-y-matsubara commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Jul 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara commented Jul 1, 2020

Uh oh!

j-y-matsubara commented Jul 1, 2020

Uh oh!

j-y-matsubara commented Jul 2, 2020

Uh oh!

mik-laj commented Jul 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara commented Jul 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-y-matsubara commented Jun 26, 2020 •

edited

Loading

j-y-matsubara Jun 27, 2020 •

edited

Loading

j-y-matsubara commented Jun 28, 2020 •

edited

Loading

j-y-matsubara commented Jun 30, 2020 •

edited

Loading

j-y-matsubara Jul 1, 2020 •

edited

Loading

j-y-matsubara Jul 1, 2020 •

edited

Loading

j-y-matsubara Jul 1, 2020 •

edited

Loading

j-y-matsubara Jul 2, 2020 •

edited

Loading

turbaszek Jul 3, 2020 •

edited

Loading

j-y-matsubara Jul 3, 2020 •

edited

Loading