Skip to content
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.

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

Salesforce Bulk API Connection + Bulk and REST Query Implementations #1404

Merged
merged 1 commit into from
Jan 8, 2016

Conversation

dlstadther
Copy link
Collaborator

Originally (and paritially implemented) by PR #981.

Most of the Salesforce Bulk API methods are implemented. Additionally, specific support for Bulk Query and REST Query have been added.

Tests are forthcoming. This PR was prematurely submitted in order to get the ball rolling with comments.

Thanks,

@erikbern
Copy link
Contributor

Looks great but great if you can fix this:

  1. the unit test fails because the "requests" module can't be imported: https://travis-ci.org/spotify/luigi/jobs/91463249 – consider adding a try: ... except ImportError: warnings.warn('need the requests module for salesforce) similar to eg https://github.com/spotify/luigi/blob/master/luigi/contrib/esindex.py
  2. The output and output_path methods are not needed on the base class so you should remove them
  3. (optional) Is there a way to unittest this? For instance you could mock requests or even run a real server on a random port.

@dlstadther
Copy link
Collaborator Author

I have updated to account for #1. Support for normal queries via the Salesforce REST Api is nearing completion.

@erikbern Could you explain #2? The output of QuerySalesforce needs to go somewhere. How can I output something within the run() without knowing whether it's going to be a LocalTarget or S3 or whatever?

Finally (regarding #3), I intend to mock test this PR. However, my testing experience is very (VERY) minimal and thus will take me awhile (and likely be poor).

@erikbern
Copy link
Contributor

erikbern commented Dec 3, 2015

For #2 – subclasses should implement output. This is how all other Task classes work in Luigi. So for instance

class MyDailySalesforceTask(QuerySalesforce):
    x = IntParameter()
    def soql(self):
        return blablabla...
    def output(self):
        return LocalTarget('salesforce-results-%d.txt' % self.x)

@erikbern
Copy link
Contributor

erikbern commented Dec 3, 2015

And then the base class should probably do output = NotImplemented

@dlstadther
Copy link
Collaborator Author

Got it (I think). Thanks @erikbern ! I have updated QuerySalesforce accordingly. REST queries still pending completion. Tests are to-be started.

@erikbern
Copy link
Contributor

erikbern commented Dec 5, 2015

There's an issue building docs because import requests fails – maybe put it in a try/except or just add it to requirements

@dlstadther
Copy link
Collaborator Author

@erikbern I have added your comments (minus the tests) to my branch, but I am waiting to push it until REST queries are complete. Currently, awaiting forum response to keep as dynamic as possible.

Sorry for such delay.

@dlstadther dlstadther changed the title Salesforce Bulk API Connection + Bulk Query Implementation Salesforce Bulk API Connection + Bulk and REST Query Implementations Dec 12, 2015
…port for SF bulk query.

Added support for REST API queries.
@dlstadther
Copy link
Collaborator Author

@erikbern It's going to be awhile before I can get around to writing tests for this; how do you suggest proceeding?

@erikbern
Copy link
Contributor

erikbern commented Jan 8, 2016

sorry, forgot about this. let's merge it!

erikbern added a commit that referenced this pull request Jan 8, 2016
Salesforce Bulk API Connection + Bulk and REST Query Implementations
@erikbern erikbern merged commit 67b39ea into spotify:master Jan 8, 2016
@dlstadther dlstadther deleted the salesforce branch March 16, 2016 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants