Added port property to CopyToTable #2561
Conversation
There was a problem hiding this comment.
As written, this is a breaking change for anyone using a subclassed version of RDBMS CopytoTable (postgres and redshift) and doesn't explicitly provide port as its own property.
Could you explain how providing host:port is uncomfortable? I'm not sold on this addition as written.
I'd be more open to a solution which optionally accepts port property. I imagine there is an example of something similar in postgres.py or redshift.py.
Happy to discuss further! Thanks for wishing to contribute back to Luigi ! :)
|
Agree, did not pay attention to that, sorry. Fixed to optional property. DB_HOST = 'localhost'
DB_NAME = 'database'
DB_USER = 'user'
DB_PASSWORD = 'password'
DB_PORT = 15432
class MyCopyToTable(CopyToTable):
host = f'{config.DB_HOST}:{config.DB_PORT}'Which looks a little bit ugly :) P.S. I was curious why |
There was a problem hiding this comment.
This is functionally kosher now.
As to why PostgresTarget and CopyToTable differ as such, I don't have a strong reason. PostgresTarget predates me. But as most luigi classes, they were added for a specific use case at a point in time. If it is your preference to refactor, i'll review.
Otherwise, i'll merge this in the next couple days
|
Is it possible to merge this one? |
|
Thanks for pinging @adilkhash . Merged! |
Description
Added abstract property
portto CopyToTableMotivation and Context
Sometimes it is not comfortable to provide port inside host property, i.e.
localhost:port, hence added optionalportproperty toCopyToTabletask in case when database listens custom port. Currently the only way to supply port is viahost.Have you tested this? If so, how?
"I ran my jobs with this code and it works for me."