8000 Update postgres.py by dhuckins · Pull Request #2615 · spotify/luigi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update postgres.py #2615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 5, 2019
Merged

Update postgres.py #2615

merged 2 commits into from
Jan 5, 2019

Conversation

dhuckins
Copy link
Contributor
@dhuckins dhuckins commented Jan 3, 2019

added port to PostgresTarget

Description

added port to the postgres.Query

Motivation and Context

was required to use postgres.Query to connect to a postgres db that did not use the default port

Have you tested this? If so, how?

"I ran my jobs with this code and it works for me."

@dhuckins
Update postgres.py
af6bbac 
added port to `PostgresTarget`
dlstadther
dlstadther reviewed Jan 4, 2019
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

LGTM - but please update test :)

@dhuckins
Copy link
Contributor Author
dhuckins commented Jan 5, 2019

not sure what failed here

@dlstadther
Copy link
Collaborator

That top codecov can be ignored. The important part is that Travis passed.

@dlstadther dlstadther merged commit 5239029 into spotify:master Jan 5, 2019
@dhuckins dhuckins deleted the patch-1 branch January 10, 2019 14:22
@duckontheweb
Copy link
Contributor

Hey, just FYI that this seems to have created a breaking change in a patch release. The previous way of defining ports was within the host property on the class, like so:

class MyQueryTask(luigi.contrib.postgres.PostgresQuery):
    host = ':'.join([db_host, db_port])
    database = db_name
    user = db_user
    password = db_password

This now raises an exception like the following:

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/luigi/worker.py", line 401, in check_complete
    is_complete = task.complete()
  File "/usr/local/lib/python3.6/site-packages/luigi/task.py", line 827, in complete
    return all(r.complete() for r in flatten(self.requires()))
  File "/usr/local/lib/python3.6/site-packages/luigi/task.py", line 827, in <genexpr>
    return all(r.complete() for r in flatten(self.requires()))
  File "/usr/local/lib/python3.6/site-packages/luigi/task.py", line 565, in complete
    outputs = flatten(self.output())
  File "/usr/local/lib/python3.6/site-packages/luigi/contrib/postgres.py", line 396, in output
    port=self.port
AttributeError: 'MyQueryTask' object has no attribute 'port'

It might be worth making this backwards compatible with the old format.

@dlstadther
Copy link
Collaborator
dlstadther commented Jan 16, 2019 •
edited
Loading

@dhuckins is this something you'd like to look into and patch?

Thanks for raising @duckontheweb

@dhuckins dhuckins mentioned this pull request Jan 16, 2019
Closed
@dhuckins
Copy link
Contributor Author

sure, added issue #2625
there's something similar

luigi/luigi/contrib/mssqldb.py

Lines 59 to 64 in eadf7ab

if ':' in host:
self.host, self.port = host.split(':')
self.port = int(self.port)
else:
self.host = host
self.port = 1433

@honnix honnix mentioned this pull request Jan 17, 2019
Merged
@honnix
Copy link
Member
honnix commented Jan 17, 2019

I think this PR is not really needed. I suggest we revert it.

@honnix
Copy link
Member
honnix commented Jan 17, 2019

cc @NatashaL

@NatashaL
Copy link
Contributor

Fine by me. Then we close #2627 too

@dhuckins
Copy link
Contributor Author

shouldn't Query and CopyToTable have the same parameters?

@honnix
Copy link
Member
honnix commented Jan 18, 2019

They should, so #2627 adds that.

@ulzha ulzha removed their request for review January 18, 2019 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlstadther dlstadther dlstadther approved these changes

@Tarrasch Tarrasch Awaiting requested review from Tarrasch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhuckins @dlstadther @duckontheweb @honnix @NatashaL
0