Skip to content

Conversation

@turbaszek
Copy link
Member

Make sure you have checked all steps below.

Jira

Description

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@mik-laj mik-laj added the provider:google Google (including GCP) related issues label Sep 24, 2019
if existing_status == 'RUNNING':
self.log.info('Cluster exists and is already running. Using it.')
return True
return existing_cluster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never looked into this operator previously, but one point that I would like to point out:
Earlier this method is return True/False, and after your change it will return mixed things (False/result from DataProcHook.find_cluster(), which can be an actual value or None). I do not find this a good practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I did this because it's a restricted method used only in execute where I handle this output. And that's better than failing operator (system tested).

Probably I will close this issue soon because I am refactoring Dataproc hook / operators.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's strange to do it this way. It should be consistently one type. Waiting for the refactor :)

@turbaszek
Copy link
Member Author

Addressed in #6371

@turbaszek turbaszek closed this Nov 5, 2019
@turbaszek turbaszek deleted the fix-dataproc2 branch December 4, 2019 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants