You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/11/27 14:18:56 UTC
[GitHub] [airflow] turbaszek opened a new pull request #12657: Add --ignore flag to upgrade_check command
turbaszek opened a new pull request #12657:
URL: https://github.com/apache/airflow/pull/12657
In some cases users may want to suppress some checks. Adding this
flag will allow them to do this.
I also disable logging to avoid output between lines of check.
Before:
```
root@5d103907dca8:/opt/airflow# airflow upgrade_check -i LegacyUIDeprecated -i ConnTypeIsNotNullableRule -i TaskHandlersMovedRule -i VersionCheckRule -i MesosExecutorRemovedRule
====================================================================================================== STATUS ======================================================================================================
Remove airflow.AirflowMacroPlugin class...................................................................................................................................................................SUCCESS
Chain between DAG and operator not allowed................................................................................................................................................................SUCCESS
Connection.conn_id is not unique..........................................................................................................................................................................SUCCESS
[2020-11-27 14:09:31,544] {__init__.py:50} INFO - Using executor LocalExecutor
[2020-11-27 14:09:31,546] {dagbag.py:417} INFO - Filling up the DagBag from /root
Ensure users are not using custom metaclasses in custom operators.........................................................................................................................................SUCCESS
Fernet is enabled by default..............................................................................................................................................................................SUCCESS
GCP service account key deprecation.......................................................................................................................................................................SUCCESS
Changes in import paths of hooks, operators, sensors and others...........................................................................................................................................SUCCESS
Logging configuration has been moved to new section.......................................................................................................................................................SUCCESS
Users must set a kubernetes.pod_template_file value.......................................................................................................................................................FAIL
SendGrid email uses old airflow.contrib module............................................................................................................................................................SUCCESS
Jinja Template Variables cannot be undefined..............................................................................................................................................................SUCCESS
Found 1 problem.
```
After:
```
root@5d103907dca8:/opt/airflow# airflow upgrade_check -i LegacyUIDeprecated -i ConnTypeIsNotNullableRule -i TaskHandlersMovedRule -i VersionCheckRule -i MesosExecutorRemovedRule
====================================================================================================== STATUS ======================================================================================================
Remove airflow.AirflowMacroPlugin class...................................................................................................................................................................SUCCESS
Chain between DAG and operator not allowed................................................................................................................................................................SUCCESS
Connection.conn_id is not unique..........................................................................................................................................................................SUCCESS
Ensure users are not using custom metaclasses in custom operators.........................................................................................................................................SUCCESS
Fernet is enabled by default..............................................................................................................................................................................SUCCESS
GCP service account key deprecation.......................................................................................................................................................................SUCCESS
Changes in import paths of hooks, operators, sensors and others...........................................................................................................................................SUCCESS
Logging configuration has been moved to new section.......................................................................................................................................................SUCCESS
Users must set a kubernetes.pod_template_file value.......................................................................................................................................................FAIL
SendGrid email uses old airflow.contrib module............................................................................................................................................................SUCCESS
Jinja Template Variables cannot be undefined..............................................................................................................................................................SUCCESS
Found 1 problem.
```
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532039706
##########
File path: airflow/upgrade/config.py
##########
@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+import yaml
+
+from airflow.utils.module_loading import import_string
+
+
+class UpgradeConfig(object):
+ def __init__(self, raw_config):
+ self._raw_config = raw_config
+
+ def remove_ignored_rules(self, rules):
+ rules_to_ignore = self._raw_config.get("ignored_rules")
+ if not rules_to_ignore:
+ return rules
+ return [r for r in rules if r.__class__.__name__ not in rules_to_ignore]
+
+ def register_custom_rules(self, rules):
+ custom_rules = self._raw_config.get("custom_rules")
+ if not custom_rules:
+ return rules
+ for custom_rule in custom_rules:
+ rule = import_string(custom_rule)
+ rules.append(rule())
+ return rules
+
+ @classmethod
+ def read(cls, path):
+ with open(path) as f:
+ raw_config = yaml.safe_load(f)
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532036258
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
+
+ logging.disable(logging.WARNING)
Review comment:
> You're going to have to explain why this line is here.
@ashb should I add comment in code?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#issuecomment-735313682
It seems that `tests/upgrade/rules/test_undefined_jinja_varaibles.py::TestUndefinedJinjaVariablesRule::test_invalid_check` is little bit flaky
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r531779100
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
+
+ logging.disable(logging.WARNING)
Review comment:
You're going to have to explain why this line is here.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532039734
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
Review comment:
Refactored the logic a little bit, let me know what you think
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532039729
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
+
+ logging.disable(logging.WARNING)
Review comment:
Please
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #12657:
URL: https://github.com/apache/airflow/pull/12657
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532040019
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
+
+ logging.disable(logging.WARNING)
Review comment:
Setting disable to `ERROR` will suppress the dagbag errors so I removed the logic from undefined jinja rule
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r531779337
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
+
+ logging.disable(logging.WARNING)
Review comment:
And if https://github.com/apache/airflow/commit/f47efe91a8b4ba5ba43fa8d55990d27516ab0b6c#diff-62e7779cba210540c1bb87f26ce56a6176941d024635c606bcf360c14b4a9e12 isn't needed anymore as a result then you should remove it
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r532009028
##########
File path: airflow/upgrade/config.py
##########
@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+import yaml
+
+from airflow.utils.module_loading import import_string
+
+
+class UpgradeConfig(object):
+ def __init__(self, raw_config):
+ self._raw_config = raw_config
+
+ def remove_ignored_rules(self, rules):
+ rules_to_ignore = self._raw_config.get("ignored_rules")
+ if not rules_to_ignore:
+ return rules
+ return [r for r in rules if r.__class__.__name__ not in rules_to_ignore]
+
+ def register_custom_rules(self, rules):
+ custom_rules = self._raw_config.get("custom_rules")
+ if not custom_rules:
+ return rules
+ for custom_rule in custom_rules:
+ rule = import_string(custom_rule)
+ rules.append(rule())
+ return rules
+
+ @classmethod
+ def read(cls, path):
+ with open(path) as f:
+ raw_config = yaml.safe_load(f)
Review comment:
Can you add JSON Schema validation? We accept user data here, so we cannot trust this data to be correct, so we should validate it.
```json
{
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"ignored_rules": {
"type": "array",
"items": {
"type": "string"
}
},
"custom_rules": {
"type": "array",
"items": {
"type": "string"
}
}
},
"additionalProperties": false
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12657: Add --ignore flag to upgrade_check command
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#issuecomment-734864409
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12657: Add possibility to configure upgrade_check command
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12657:
URL: https://github.com/apache/airflow/pull/12657#discussion_r531779661
##########
File path: airflow/upgrade/checker.py
##########
@@ -45,19 +46,44 @@ def register_arguments(subparser):
"-s", "--save",
help="Saves the result to the indicated file. The file format is determined by the file extension."
)
+ subparser.add_argument(
+ "-i", "--ignore",
+ help="Ignore a rule. Can be used multiple times.",
+ action='append',
+ )
+ subparser.add_argument(
+ "-c", "--config",
+ help="Path to upgrade check config yaml file.",
+ )
subparser.set_defaults(func=run)
def run(args):
from airflow.upgrade.formatters import (ConsoleFormatter, JSONFormatter)
+ from airflow.upgrade.config import UpgradeConfig
+
if args.save:
filename = args.save
if not filename.lower().endswith(".json"):
exit("Only JSON files are supported")
formatter = JSONFormatter(args.save)
else:
formatter = ConsoleFormatter()
- all_problems = check_upgrade(formatter)
+
+ if args.ignore:
+ rules = [r for r in ALL_RULES if r.__class__.__name__ not in args.ignore]
+ else:
+ rules = ALL_RULES
+
+ if args.config:
+ print("Using config file from:", args.config)
+ upgrade_config = UpgradeConfig.read(path=args.config)
+ rules = upgrade_config.register_custom_rules(rules)
+ rules = upgrade_config.remove_ignored_rules(rules)
Review comment:
How about you make `remove_ignored_rules` a function in here, and then it can be used for both `args.ignore` and the value from the config file.
----------------------------------------------------------------
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:
users@infra.apache.org