potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow URL: https://github.com/apache/airflow/pull/7191#discussion_r368761263
########## File path: tests/contrib/operators/test_singularity_operator.py ########## @@ -0,0 +1,180 @@ +# -*- coding: utf-8 -*- +# +# 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. + +import unittest +import six + +from parameterized import parameterized +from spython.instance import Instance +from airflow import AirflowException + +try: + from airflow.contrib.operators.singularity_operator import SingularityOperator +except ImportError: + pass Review comment: > This is not logical to me - both of these files have a newline at the end, yet they are reported fixed. It's rather logical IMHO - I think you are likely thinking about empty LINE at the end, but what the end-of-file rightfully complained about was extra EOL character - empty line and EOL character are two different things. The end-of-file fixer will fail if it has either any number of empty lines at the end or lack of EOL character at the end of last line. Both cases are undesirable for various reasons (you cannot easily append to a file which has no EOL and extra empty line is superfluous and might hide/miss the true end of file in editor. That's why it failed likely because you had an empty line at the end (technically empty line at the end is exactly TWO EOL characters). It expects exactly one EOL at the end of file - no more, no less. > The command you provided didn't work First of all - you did not run it with `--all-files` . That's why it did not fix your files - they were already committed, not staged. As explained in our "Use pre-commits" documentation, `pre commit run` only works on staged files. In your case you tried to run in on already committed files. For that `pre-commit run --all-files` is appropriate because it runs it on all files, not only on staged files. That should have fixed your problem with automated fixing of the files. I even mentioned both of the versions in my previous comment. For the docker build failure - you were extremely unlucky in this case. There was a release of an extra dependency that broke the master build (not our fault) - that's why you had the sql.h error. It's been fixed in one of the latest commit but you are currently behind by 26 commits. You should rebase your change to the latest master to get it working. You should do that anyway before merge (the best idea is to rebase often). You can see an exact way how to rebase in our documentation here: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#how-to-rebase-pr ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
