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

Hubspot Source #732

Merged
merged 10 commits into from
Oct 30, 2020
Merged

Hubspot Source #732

merged 10 commits into from
Oct 30, 2020

Conversation

cgardens
Copy link
Contributor

What

  • Add hubspot source.
  • Just uses standard tests
  • Hubspot supports to sets of auth api key and oauth. tap-hubspot supports both and so do we.

Timer

  • 85 minutes: to get it done with just api key
  • additional 67 minutes: to get it done with oauth too (though 60% of that was learning where hubspot hid the creds in their ui)
  • 25 minutes: to do clean up and docs.
  • total 175 minutes (~3 hours)

Where did the time go

  • Doing oauth is always a hassle and just takes time to go through integration docs and get it right. I'm not sure there's much to be done to make this faster. With practice, I'm sure I'll get faster at this, but not a code thing to do.
  • I was just running it against the standard tests until it worked. This was good except since all the docker images build every time, it means the iteration speed is like a minute and half. If we fix the caching here, this should become pretty reasonable.
  • A lot of the initial time was spent trying renaming files, directors, class names, etc. This should all go away with the template.

Release

  • Blocked on being able to render oneOf in the ui.

Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

% my question about refresh token.

1. Access the api key credentials in the `hubspot-integration-test-api-key` secret on Rippling under the `Engineering` folder
1. If we ever need a new api key it can be found in settings -> integrations (under the account banner) -> api key
1. Access the oauth config in the `hubspot-integration-test-oauth-config` secret on Rippling under the `Engineering` folder
1. If we ever need to regenerate the refresh token for auth follow these hubspot [instructions](https://developers.hubspot.com/docs/api/oauth-quickstart-guide). Going to lay out the process because it wasn't 100% clear in their docs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the refresh token something that need to be generated from the integration itself instead of an input?

Copy link
Contributor Author

@cgardens cgardens Oct 29, 2020

Choose a reason for hiding this comment

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

Ah, as in could we do the oauth within our UI and generate the refresh_token ourselves? Good thought. 2 things:

  1. It is not uncommon for singer taps / targets to require a refresh token. That's because there's no UI for it to do a normal oauth flow so it cheeses the oauth flow. It seems fairly standard in singer world. (both the tap-salesforce and tap-hubspot use this pattern).
  2. Since we have the UI, we don't necessarily need to be bound by this constraint, but we would have to add a new feature to the UI to support doing oauth and add this to the integration interface. I think I'd be in favor of doing this later, since:
    1. it's always already common to do it the current way in these tools and
    2. will be a non trivial change in both the ui and integration iface
    3. it will only make it easier to use our integrations, we'll just remove refresh_token from configs, so it's an easy migration for users and easy for us to maintain backwards compatibility.

does this address what you were thinking of? or am i misunderstanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does address my question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to track us supporting oauth in a more use-friendly way: #768

@@ -0,0 +1,24 @@
plugins {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need java?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't! thank you.

@cgardens cgardens merged commit edc49b8 into master Oct 30, 2020
@cgardens cgardens deleted the cgarden/hubspot branch October 30, 2020 05:05
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.

3 participants