-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Source Greenhouse #1076
Source Greenhouse #1076
Conversation
f9a475a
to
30520ad
Compare
based on #1068 - not anymore, rebased on master |
bb9c70e
to
8aa0c1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty close to shipping, the main two things remaining are:
- merge master to pick up the changes in this PR Freshdesk fixes #1145 and resolve any conflicts
- run
./tools/integrations/manage.sh build airbyte-integrations/connectors/source-greenhouse
from the repo root and verify the tests are passing
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
|
||
# FIXME: patch missing endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this comment about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current Greenhouse package doesn't support custom_fields
endpoint, so I have to patch internals manually here. I have created a PR for the Greenhouse package, but it doesn't seem to be very much alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have improved the comment to clarify this
self._uris["direct"]["custom_fields"] = {"list": "custom_fields", "retrieve": "custom_field/{id}"} | ||
|
||
def __getattr__(self, key): | ||
# FIXME: patch auth object to avoid bug with serialization of None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this suppsed to be done in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the latest package has an issue that is fixed in the master branch, we just need it to be released to drop this line
@@ -0,0 +1,45 @@ | |||
# Greenhouse | |||
|
|||
## Overview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
airbyte-integrations/connectors/source-greenhouse/Dockerfile.test
Outdated
Show resolved
Hide resolved
|
to use it append `&template=source.md` to standard MR URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- will merge in the morning
…n-usable-stream Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
What
Add new source Greenhouse
How
Describe the solution
Checklist
Recommended reading order
test.java
component.ts