fix(source-intercom) raise config error on Active subscription needed error, transient error for Companies stream

[darynaishchenko]

What

resolved: https://github.com/airbytehq/oncall/issues/2909, https://github.com/airbytehq/oncall/issues/5291

How

Updated the source to use latest aribyte cdk.
Added config error for Active subscription needed error
Updated error handling for stream Companies

Review guide

User Impact

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jul 29, 2024 9:21am

[darynaishchenko]

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (bf31e13)

[brianjlai]

Things look good to me on the version bump and the fix for the error. One thing I wanted to highlight is on my OC shift, I noticed an issue related to intercom that could also be addressed in the version bump.

https://github.com/airbytehq/oncall/issues/5291 is asking to update recent CDK changes and classify the 400 and 500 errors as non-system errors. These are both transient due to Intercom or the customer's configuration along with a custom error.

Do you mind also updating the response_filters on the companies stream to allow for applying the correct failure type and error message to also close out this OC issue?

error_handlers:
  - type: DefaultErrorHandler
    description:
      " 400 - existing scroll_param, need to wait at least 60 sec
      to continue and retry 500 - server-side error, should retry after 60
      sec. "
    response_filters:
    < changes to manifest start here >
      - http_codes: [400]
        action: RETRY
        failure_type: transient_error
        error_message: "A scroll (export) job is already in progress for this Intercom account, causing the request to fail. Only one active scroll per Intercom account is allowed; ensure no overlap by limiting active connections or scheduling jobs appropriately."
      - http_codes: [500]
        action: RETRY
        failure_type: transient_error
    < changes to manifest end here >
    backoff_strategies:
      - type: ConstantBackoffStrategy
        backoff_time_in_seconds: 60
  - type: DefaultErrorHandler
    description:
      "404 - scroll_param is expired or not found while requesting,
      ignore"
    response_filters:
      - http_codes: [404]
        action: IGNORE

[darynaishchenko]

@brianjlai Sure, I'll update it. UPD: updated error handling for companies stream

[darynaishchenko]

Regression tests report:
test_catalog_are_the_same [failed]: new connector version contains new fields: "is_resumable": true; and "multipleOf": 1e-08 now is "multipleOf": 1e-8

[brianjlai]

thanks for updating the connector. I will re-review Daryna

the is_resumable should be expected since we're bumping past CDK ^1 where that change to the catalog was added. We can mark that as an acceptable "regression" and it is used for incremental syncs + rfr syncs.

I am not sure about the second one however

[brianjlai]

question about whether we still need a custom Intercom HttpRequester class

[brianjlai]

Something that seems unclear to me is where this custom IntercomHttpRequester differs from the regular HttpRequester that is supplied by the low-code CDK, since we removed interpret_response_status into a custom error handler, it doesn't seem like this class is overriding anything meaningful.

I see that we instantiate a bunch of interpolators, but the overwritten get_request_params() and other methods seem to just do the same behavior as interpolated_request_options_provider.py which makes me think this class is not needed anymore.

What do you think @darynaishchenko ? If this is in fact needed, let's add a docstring explaining the need for this custom class. Otherwise we should just swap this out and use HttpRequester in the manifest

[darynaishchenko]

Yes, you are right, here is no need to overwrite HttpRequester now. Changed it manifest file.
Run regression tests here: request urls looks good.

[darynaishchenko]

thanks for updating the connector. I will re-review Daryna

the is_resumable should be expected since we're bumping past CDK ^1 where that change to the catalog was added. We can mark that as an acceptable "regression" and it is used for incremental syncs + rfr syncs.

I am not sure about the second one however

@brianjlai Also not sure about second one, it should affect data if we use default schema normalization, but it's disabled for intercom. I didn't find any usage of it in destinations, so looks like it was added for documentation purpose. I think we can move on with it.

[pnilan]

Looks good! nice work.

[brianjlai]

nice work @darynaishchenko and thanks for wrapping in the additional error handling change. that should also wrap an OC issue we have out right now

[darynaishchenko]

regression tests for latest version: link
test_catalog_are_the_same [failed] with the same error above.

"multipleOf": 1e-08 now is "multipleOf": 1e-8

I double checked value for multipleOf field in the get response for refresh schema action on UI and on both version it's 1.0e-8(0.00000001 as manifest declares).
So I'm approving regression tests for these changes.

[darynaishchenko]

/approve-regression-tests

Check job output.

✅ Approving regression tests