Skip to content

Conversation

@JavierLopezT
Copy link
Contributor

@JavierLopezT JavierLopezT commented Nov 26, 2019

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Adding mysqltoS3Operator

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:amazon AWS/Amazon - related issues label Nov 27, 2019
@potiuk
Copy link
Member

potiuk commented Dec 3, 2019

FYI @JavierLopezT . the static check problems are being fixed in #6596 so please rebase after it's merged (soon).

@OmerJog
Copy link
Contributor

OmerJog commented Dec 3, 2019

There is also a need to add test to this new operator, is it not?

@JavierLopezT
Copy link
Contributor Author

JavierLopezT commented Dec 3, 2019

@OmerJog Indeed; I just wanted to have the operator itself finished before coding the test

@JavierLopezT
Copy link
Contributor Author

JavierLopezT commented Dec 12, 2019

I am unable to solve the next error in the documentation:

/opt/airflow/docs/_api/airflow/operators/mysql_to_s3_operator/index.rst:36: WARNING: Unexpected indentation.
/opt/airflow/docs/_api/airflow/operators/mysql_to_s3_operator/index.rst:38: WARNING: Block quote ends without a blank line; unexpected unindent.

Could anyone help me solving this please? Thank you very much

@JavierLopezT
Copy link
Contributor Author

I don't know how to solve the test errors. I understand that it is something related to the SQL session. However I don't know how to fix it, as per my understanding, the mock object is executing the get_df from MySQL Hook. Could anyone guide me a little please? Thank you very much

@codecov-io
Copy link

Codecov Report

Merging #6670 into master will increase coverage by 0.77%.
The diff coverage is 75.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6670      +/-   ##
=========================================
+ Coverage   83.92%   84.7%   +0.77%     
=========================================
  Files         668     679      +11     
  Lines       37722   38518     +796     
=========================================
+ Hits        31660   32627     +967     
+ Misses       6062    5891     -171
Impacted Files Coverage Δ
...ow/contrib/example_dags/example_qubole_operator.py 80% <ø> (ø) ⬆️
...ample_dags/example_branch_python_dop_operator_3.py 75% <ø> (ø) ⬆️
...flow/contrib/example_dags/example_qubole_sensor.py 100% <ø> (ø) ⬆️
airflow/contrib/hooks/qubole_hook.py 52.67% <ø> (ø) ⬆️
airflow/hooks/hive_hooks.py 77.6% <ø> (ø) ⬆️
...low/example_dags/example_short_circuit_operator.py 100% <ø> (ø) ⬆️
...ample_dags/example_emr_job_flow_automatic_steps.py 100% <ø> (ø) ⬆️
...ow/gcp/operators/cloud_storage_transfer_service.py 95.63% <ø> (ø) ⬆️
...contrib/example_dags/example_papermill_operator.py 100% <ø> (ø) ⬆️
airflow/example_dags/example_http_operator.py 100% <ø> (ø) ⬆️
... and 245 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f040fc...21a058a. Read the comment docs.

@JavierLopezT
Copy link
Contributor Author

Ok. I have messed up including all the commits that were made in airflow for the past days in this PR. Should I close this PR and open a new one to clean the mess? I am sorry

@potiuk
Copy link
Member

potiuk commented Jan 3, 2020

No worries :). You can simply rebase the last commit onto the apache master. If you have apache repo added as remote then this will work:

git fetch --all
git rebase HEAD^ --onto apache/master

To rebase only last commit onto the latest apache/master

@JavierLopezT
Copy link
Contributor Author

No worries :). You can simply rebase the last commit onto the apache master. If you have apache repo added as remote then this will work:

git fetch --all
git rebase HEAD^ --onto apache/master

To rebase only last commit onto the latest apache/master

Thank you very much. I have apache added as remote upstream and I my personal repo as remote origin. I have made rebase --onto upstream/master but it makes nothing
Screen Shot 2020-01-07 at 9 46 36 AM
I have tried also with fetching several times, with --onto origin/master and apache/master (which gives me a fatal: Does not point to a valid commit 'apache/master' error)
What could I do? Thank you very much

@potiuk
Copy link
Member

potiuk commented Jan 7, 2020

But it does what it is supposed to do :. Once you rebased the code --onto upstream/master you get conflict. Then you should do exactly what the message says:

  1. You need to resolve the conflict: see for example this thread: how to do it https://stackoverflow.com/questions/161813/how-to-resolve-merge-conflicts-in-git but you can also use Pycharm/IntelliJ's fantastic VCS -> Git -> Resolve Conflicts tools.
  2. Once you resolve the conflict and 'git add' all the changes resulting from that you need to do git rebase --continue (this is what the message suggests to do.
  3. Once you are done you can git push -f the change to your master

That's it.

The important thing is that you must resolve conflict - this is usual. If you have different people modifying the same files, you get conflicts and you need to learn to solve them.

@potiuk
Copy link
Member

potiuk commented Jan 7, 2020

What you did you "skipped" the coflict instead so it changed nothing indeed.

@JavierLopezT
Copy link
Contributor Author

Could anyone help me a little more with the unittest please? Thank you very much in advance

@turbaszek turbaszek requested a review from feluelle January 28, 2020 15:31
@JavierLopezT
Copy link
Contributor Author

Thanks for your suggestions @feluelle They are pushed already

@turbaszek turbaszek requested a review from feluelle February 8, 2020 07:18
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

Ouch, the mock has to be initialized before calling execute...

@feluelle
Copy link
Member

Please rebase onto latest master. We had an issue that is fixed now.

@JavierLopezT JavierLopezT mentioned this pull request Jun 10, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants