Source Salesforce: update regex pattern, specify format to date-time #24071

arsenlosenko · 2023-03-14T20:06:31Z

What

User is not able to set start_date properly during setup of Source Salesforce. See slack thread for more context

How

Remove regex pattern that was used previously, specify date-time as format, use it for validation.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

arsenlosenko · 2023-03-14T20:07:21Z

/test connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/4419773337
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/4419773337
Python tests coverage:

Name                                         Stmts   Miss  Cover
----------------------------------------------------------------
source_salesforce/utils.py                       8      0   100%
source_salesforce/__init__.py                    2      0   100%
source_salesforce/source.py                     96      6    94%
source_salesforce/streams.py                   398     32    92%
source_salesforce/api.py                       155     14    91%
source_salesforce/exceptions.py                  8      1    88%
source_salesforce/rate_limiting.py              22      3    86%
source_salesforce/availability_strategy.py      17      8    53%
----------------------------------------------------------------
TOTAL                                          706     64    91%
Name                                         Stmts   Miss  Cover
----------------------------------------------------------------
source_salesforce/__init__.py                    2      0   100%
source_salesforce/exceptions.py                  8      1    88%
source_salesforce/api.py                       155     21    86%
source_salesforce/availability_strategy.py      17      3    82%
source_salesforce/streams.py                   398     86    78%
source_salesforce/rate_limiting.py              22      6    73%
source_salesforce/source.py                     96     33    66%
source_salesforce/utils.py                       8      7    12%
----------------------------------------------------------------
TOTAL                                          706    157    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
================== 37 passed, 1 skipped in 636.35s (0:10:36) ===================

erohmensing · 2023-03-14T20:27:53Z

Hm, is this a breaking change for connectors that were set up with the `"2021-07-25" format?

Maybe adding the format: date was a breaking change to users who were configured using the date-time format too?

Or do we not re-validate that the config matches before we run syncs? (we do however run check again on failed syncs, so that would definitely fail I think)

erohmensing · 2023-03-15T15:49:36Z

@arsenlosenko do you know the answer to my question above?

arsenlosenko · 2023-03-15T15:56:01Z

@erohmensing sorry for the late response, I don't have an answer to your question right now, let me check if this breaks anything once again. Last night I tested sync locally with format both set to date and date-time and it worked, but I'll test this once again for good measure

erohmensing · 2023-03-15T15:58:11Z

@arsenlosenko thanks! By trying it with both do you mean you kept the format: whatever the same in the spec, and tried passing both values? Or you changed the spec each time?

arsenlosenko · 2023-03-15T16:00:07Z

@erohmensing I changed the spec each time, but let me try to pass different values with the same format value in spec, this way we could see if this affects anything

erohmensing · 2023-03-15T16:29:55Z

@artem1205 unfortunately testing with the spec changed won't reproduce the actual behavior since only one version of the spec can be canonical at once.

We should also test check in addition to the sync as I think this is where it was failing

artem1205 · 2023-03-15T16:35:53Z

cc @arsenlosenko

arsenlosenko · 2023-03-15T17:25:45Z

@erohmensing @artem1205 I am still
testing check and sync with different formats of date in spec, I'll get back to you once I have the results

erohmensing · 2023-03-15T18:00:22Z

Ope, sorry for the incorrect ping @artem1205!

airbyte-integrations/connectors/source-salesforce/source_salesforce/spec.yaml

grubberr · 2023-03-15T19:28:10Z

@arsenlosenko do we need fix this https://github.com/airbytehq/airbyte/blob/master/docs/integrations/sources/salesforce.md?plain=1#L51

YYYY-MM-DD

?

arsenlosenko · 2023-03-15T19:36:20Z

@grubberr I want to check if we can use different date formats, but as you indicated, in docs there is only one specified date format, compared to examples in spec, which are in different format. I am not that familiar with this connector, and don't know if this date/date-time will affect the performance of the source.
If we are relying only on description of this field in docs, then yes, we can set format to date, remove example of datetime from spec, and update regex pattern.

bnchrch · 2023-03-15T19:57:12Z

Sorry for a delayed review.

I wanted to see what current values we had for start_date in prod:

Unfortunately we have both date, datetime and null.

Double checking the regex provided shows that it matches both cases

So it looks like we should keep the permissive regex and extra example

bnchrch

Approving as it seems like this is the correct way to do pattern validation at this fime. (See previous comment)

Before publishing and merging @arsenlosenko you can confirm that spec, check and sync are working for both example formats?

arsenlosenko · 2023-03-15T20:05:28Z

@bnchrch thanks for the review and clarification 👍
And yes,I will check if connector performs as expected with both date formats before publishing and and merging it

…-fix

arsenlosenko · 2023-03-16T08:52:20Z

/test connector=connectors/source-salesforce

🕑 connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/4435190820
✅ connectors/source-salesforce https://github.com/airbytehq/airbyte/actions/runs/4435190820
Python tests coverage:

Name                                         Stmts   Miss  Cover
----------------------------------------------------------------
source_salesforce/utils.py                       8      0   100%
source_salesforce/__init__.py                    2      0   100%
source_salesforce/source.py                     96      6    94%
source_salesforce/streams.py                   398     32    92%
source_salesforce/api.py                       155     14    91%
source_salesforce/exceptions.py                  8      1    88%
source_salesforce/rate_limiting.py              22      3    86%
source_salesforce/availability_strategy.py      17      8    53%
----------------------------------------------------------------
TOTAL                                          706     64    91%
Name                                         Stmts   Miss  Cover
----------------------------------------------------------------
source_salesforce/__init__.py                    2      0   100%
source_salesforce/exceptions.py                  8      1    88%
source_salesforce/api.py                       155     21    86%
source_salesforce/availability_strategy.py      17      3    82%
source_salesforce/streams.py                   398     86    78%
source_salesforce/rate_limiting.py              22      6    73%
source_salesforce/source.py                     96     33    66%
source_salesforce/utils.py                       8      7    12%
----------------------------------------------------------------
TOTAL                                          706    157    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
================== 37 passed, 1 skipped in 535.89s (0:08:55) ===================

arsenlosenko · 2023-03-16T08:58:26Z

@bnchrch I have tested setup of connector locally with different date formats, using only date-time format in spec, works as expected.

Check, spec and sync works as well, will proceed to publish and merge after tests and CI are green

…-fix

arsenlosenko · 2023-03-16T14:30:07Z

/publish connector=connectors/source-salesforce

🕑 Publishing the following connectors:
connectors/source-salesforce
https://github.com/airbytehq/airbyte/actions/runs/4438192395

Connector	Did it publish?	Were definitions generated?
connectors/source-salesforce	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

…irbytehq#24071) * Source Salesforce: remove regex pattern, specify format to date-time * Update changelog * Automated Change * Add updated regex pattern for additional validation * Automated Change * auto-bump connector version --------- Co-authored-by: arsenlosenko <arsenlosenko@users.noreply.github.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

…24071) * Source Salesforce: remove regex pattern, specify format to date-time * Update changelog * Automated Change * Add updated regex pattern for additional validation * Automated Change * auto-bump connector version --------- Co-authored-by: arsenlosenko <arsenlosenko@users.noreply.github.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

Source Salesforce: remove regex pattern, specify format to date-time

8fce35c

arsenlosenko requested review from erohmensing and bazarnov March 14, 2023 20:06

arsenlosenko self-assigned this Mar 14, 2023

octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/salesforce labels Mar 14, 2023

Update changelog

46956d0

arsenlosenko and others added 2 commits March 14, 2023 20:30

Automated Change

73616f2

Add updated regex pattern for additional validation

8e9c0e4

bazarnov approved these changes Mar 15, 2023

View reviewed changes

bnchrch requested review from bnchrch and removed request for erohmensing March 15, 2023 16:16

grubberr suggested changes Mar 15, 2023

View reviewed changes

airbyte-integrations/connectors/source-salesforce/source_salesforce/spec.yaml Show resolved Hide resolved

bnchrch approved these changes Mar 15, 2023

View reviewed changes

Merge branch 'master' into arsenlosenko/source-salesforce-format-date…

9660857

…-fix

Automated Change

1b02d7c

grubberr approved these changes Mar 16, 2023

View reviewed changes

arsenlosenko changed the title ~~Source Salesforce: remove regex pattern, specify format to date-time~~ Source Salesforce: update regex pattern, specify format to date-time Mar 16, 2023

Merge branch 'master' into arsenlosenko/source-salesforce-format-date…

b90199a

…-fix

auto-bump connector version

274fb50

arsenlosenko merged commit 13bdee0 into master Mar 16, 2023
20 checks passed

arsenlosenko deleted the arsenlosenko/source-salesforce-format-date-fix branch March 16, 2023 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Salesforce: update regex pattern, specify format to date-time #24071

Source Salesforce: update regex pattern, specify format to date-time #24071

arsenlosenko commented Mar 14, 2023 •

edited by erohmensing

arsenlosenko commented Mar 14, 2023 •

edited by github-actions bot

erohmensing commented Mar 14, 2023 •

edited

erohmensing commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

artem1205 commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

grubberr commented Mar 15, 2023 •

edited

arsenlosenko commented Mar 15, 2023

bnchrch commented Mar 15, 2023

bnchrch left a comment

arsenlosenko commented Mar 15, 2023

arsenlosenko commented Mar 16, 2023 •

edited by github-actions bot

arsenlosenko commented Mar 16, 2023

arsenlosenko commented Mar 16, 2023 •

edited by github-actions bot

Source Salesforce: update regex pattern, specify format to date-time #24071

Source Salesforce: update regex pattern, specify format to date-time #24071

Conversation

arsenlosenko commented Mar 14, 2023 • edited by erohmensing

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

arsenlosenko commented Mar 14, 2023 • edited by github-actions bot

Build Passed

erohmensing commented Mar 14, 2023 • edited

erohmensing commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

artem1205 commented Mar 15, 2023

arsenlosenko commented Mar 15, 2023

erohmensing commented Mar 15, 2023

grubberr commented Mar 15, 2023 • edited

arsenlosenko commented Mar 15, 2023

bnchrch commented Mar 15, 2023

bnchrch left a comment

Choose a reason for hiding this comment

arsenlosenko commented Mar 15, 2023

arsenlosenko commented Mar 16, 2023 • edited by github-actions bot

Build Passed

arsenlosenko commented Mar 16, 2023

arsenlosenko commented Mar 16, 2023 • edited by github-actions bot

arsenlosenko commented Mar 14, 2023 •

edited by erohmensing

arsenlosenko commented Mar 14, 2023 •

edited by github-actions bot

erohmensing commented Mar 14, 2023 •

edited

grubberr commented Mar 15, 2023 •

edited

arsenlosenko commented Mar 16, 2023 •

edited by github-actions bot

arsenlosenko commented Mar 16, 2023 •

edited by github-actions bot