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/06/21 19:10:51 UTC
[GitHub] [airflow] mik-laj opened a new pull request #9467: [WIP] Add airflow upgrade-check command (MVP)
mik-laj opened a new pull request #9467:
URL: https://github.com/apache/airflow/pull/9467
This is a DRAFT and I still have to complete the missing documentation and unit tests.
I wanted to prepare a minimal valuable product for the upgrade_check command. Only one rule is implemented, but I think that's enough to show the feature. In the next steps, we can think about what rules we need to create, create tickets, and then work on the next rules by the whole community.
Recently, we have a lot of new beginner contributors and I think we can try to engage them for this task. Most rules will be small and independent, so they will be great candidates for good-first-issue.
An example of the result of this command is presented in the termianlshot below
<img width="479" alt="Screenshot 2020-06-13 at 12 58 38" src="https://user-images.githubusercontent.com/12058428/84566968-a63cc200-ad75-11ea-870b-60d07d0c44fa.png">
Example JSON:
```
[
{
"rule": "ConnTypeIsNotNullableRule",
"message": "Connection<id=''\", conn_id=AA> have empty conn_type field."
}
]
```
Part of: https://github.com/apache/airflow/issues/8765
Continuation of the change, but for 1.10: https://github.com/apache/airflow/pull/9276
---
Make sure to mark the boxes below before creating PR: [x]
- [X] Description above provides context of the change
- [X] Unit tests coverage for changes (not needed for documentation changes)
- [X] Target Github ISSUE in description if exists
- [X] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [X] Relevant documentation is updated including usage instructions.
- [XI will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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] dimberman commented on pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-687256354
When it comes to the kubernetesexecutor migrations, should I add them here or should I wait till this is merged?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#discussion_r483854376
##########
File path: airflow/bin/cli.py
##########
@@ -2151,6 +2153,19 @@ def info(args):
print(info)
+def upgrade_check(args):
+ if args.save:
+ filename = args.save
+ if not filename.lower().endswith(".json"):
+ print("Only JSON files are supported", file=sys.stderr)
+ formatter = JSONFormatter(args.save)
+ else:
+ formatter = ConsoleFormatter()
+ all_problems = check_upgrade(formatter)
+ if all_problems:
+ exit(1)
Review comment:
```suggestion
sys.exit(1)
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#discussion_r486331456
##########
File path: airflow/contrib/hooks/gcp_dataproc_hook.py
##########
@@ -337,7 +337,7 @@ def cancel(self, project_id, job_id, region='global'):
projectId=project_id,
region=region,
jobId=job_id
- )
+ ).execute(num_retries=self.num_retries)
Review comment:
yeah this one
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-690182804
Ok, no idea how to make the test runnable on CI. Two days I had only py3.5 failing, know everything is cancelled... @potiuk any ideas?
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #9467:
URL: https://github.com/apache/airflow/pull/9467
----------------------------------------------------------------
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] dimberman commented on pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-693757333
@turbaszek @mik-laj is this still a draft? Should I merge this into 1.10.13?
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-687099853
Still, some unit tests should be added but otherwise, I think it's good to get a review to validate the approach.
Basically to implement a new rule we had to create a class that inherits from `airflow.upgrade.rules.base_rule.BaseRule`. It will be auto-registered and used by `airflow upgrade-check` command. The `CustomRuleClass` has to have `title`, `description` properties and should implement `check` method which returns a list of error messages in case of incompatibility.
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#discussion_r484342592
##########
File path: airflow/upgrade/formatters.py
##########
@@ -0,0 +1,114 @@
+# 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 abc import ABCMeta
+from typing import List
+import json
+
+from airflow.upgrade.problem import RuleStatus
+from airflow.upgrade.rules.base_rule import BaseRule
+from airflow.utils.cli import header, get_terminal_size
+
+
+class BaseFormatter(object):
+ __metaclass__ = ABCMeta
+
+ def start_checking(self, all_rules):
+ # type: (List[BaseRule]) -> None
+ pass
+
+ def end_checking(self, rule_statuses):
+ # type: (List[RuleStatus]) -> None
+
+ pass
+
+ def on_next_rule_status(self, rule_status):
+ # type: (RuleStatus) -> None
+ pass
+
+
+class ConsoleFormatter(BaseFormatter):
+ def start_checking(self, all_rules):
+ print()
+ header("STATUS", "=")
+ print()
+
+ def end_checking(self, rule_statuses):
+ if rule_statuses:
+ messages_count = sum(
+ len(rule_status.messages)
+ for rule_status in rule_statuses
+ )
+ if messages_count == 1:
+ print("Found {} problem.".format(messages_count))
+ else:
+ print("Found {} problems.".format(messages_count))
+ print()
+ header("RECOMMENDATIONS", "=")
+ print()
+
+ self.display_recommendations(rule_statuses)
+ else:
+ print("Not found any problems. World is beautiful. ")
+ print("You can safely update Airflow to the new version.")
+
+ @staticmethod
+ def display_recommendations(rule_statuses):
+
+ for rule_status in rule_statuses:
+ rule = rule_status.rule
+ print(rule.title)
+ print("-" * len(rule.title))
+ print(rule.description)
+ print("")
+ if rule_status.messages:
+ print("Problems:")
+ for message_no, message in enumerate(rule_status.messages, 1):
+ print('{:>3}. {}'.format(message_no, message))
+
+ def on_next_rule_status(self, rule_status):
+ status = "SUCCESS" if rule_status.is_success else "FAIL"
+ status_line_fmt = self.prepare_status_line_format()
+ print(status_line_fmt.format(rule_status.rule.title, status))
+
+ @staticmethod
+ def prepare_status_line_format():
+ _, terminal_width = get_terminal_size()
+
+ return "{:.<" + str(terminal_width - 10) + "}{:.>10}"
+
+
+class JSONFormatter(BaseFormatter):
+ def __init__(self, output_path):
+ self.filename = output_path
+
+ def start_checking(self, all_rules):
+ print("Start looking for problems.")
+
+ @staticmethod
+ def _info_from_rule_status(rule_status):
+ return {
+ "rule": type(rule_status.rule).__name__,
+ "title": rule_status.rule.title,
+ "messages": rule_status.messages,
+ }
+
+ def end_checking(self, rule_statuses):
+ formatted_results = [self._info_from_rule_status(rs) for rs in rule_statuses]
+ with open(self.filename, "w+") as output_file:
+ json.dump(formatted_results, output_file, indent=2)
+ print("Saved result to: {}".format(self.filename))
Review comment:
Example result
```
[
{
"rule": "MokcRule",
"title": "title",
"messages": [
"msg1",
"msg2"
]
}
]
```
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-694311660
@dimberman I think we should include to in "last" 1.10 release. Currently there's only one rule implemented so not a big gain
----------------------------------------------------------------
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 pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-687401568
We want to build this tool together with the community. When this change is merged, we will create several tickets for other contributors. It will also be a good time to add a migration for k8s.
----------------------------------------------------------------
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 #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-690254313
@mik-laj @kaxil @dimberman @ashb please take a look, the CI is green and I think we are good to merge 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] turbaszek commented on pull request #9467: Add airflow upgrade-check command (MVP) (v1-10-test)
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #9467:
URL: https://github.com/apache/airflow/pull/9467#issuecomment-690266563
Thanks @ashb, I will just rebase the PR as I spotted the Dataproc HTTP commit 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