Expose cron scheduling in the Connections APIs #15253

mfsiega-airbyte · 2022-08-03T17:32:13Z

What

Expose cron scheduling in the connection APIs.

How

Introduce the scheduleType and scheduleData fields alongside the now-deprecated schedule.
Consume these if available.
Populate both the old and new schemas for the existing types, so the the frontend can switch without disruption.

🚨 User Impact 🚨

None

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Tests

Unit

Updated unit tests for connections handler
Added unit tests for cron parsing from Create requests

Integration

Todo in a subsequent PR.

Acceptance

Todo in a subsequent PR.

terencecho

overall lgtm. Added a few comments to the PR.

airbyte-api/src/main/openapi/config.yaml

airbyte-server/src/main/java/io/airbyte/server/handlers/ConnectionsHandler.java

airbyte-server/src/test/java/io/airbyte/server/helpers/ConnectionHelpers.java

Co-authored-by: terencecho <terence@airbyte.io>

…ionHelpers.java Co-authored-by: terencecho <terence@airbyte.io>

…impl

terencecho · 2022-08-05T22:40:56Z

octavia-cli/octavia_cli/apply/resources.py

+            if "cron" in configuration["schedule_data"]:
+                cron = ConnectionScheduleDataCron(**configuration["schedule_data"]["cron"])
+                configuration["schedule_data"]["cron"] = cron
+            configuration["schedule_data"] = ConnectionScheduleData(**configuration["schedule_data"])


is there a reason why the manual type isn't here?

When the type is manual, then schedule_data is null, so we only need to handle the schedule_type (in line 606).

mfsiega-airbyte · 2022-08-09T10:24:09Z

Believe any remaining test failures are unrelated to this PR.

Adding @alafanechere to take a look at the octavia-cli changes. Also a question for you - it took a bit of time to figure out what code change needed to happen, though I think this will probably happen anytime anybody adds a new object field to an API (maybe?). Any thoughts on a possible doc update to make it easier? Perhaps just a note in the README? (Though I'm not 100% sure what it should say.)

And @timroes if you want to take a quick look from the API/FE perspective?

alafanechere

@mfsiega-airbyte I think I made the appropriate changes to the CLI to manage the two new fields schedule_type and schedule_data in the generate, apply and import commands. For simplicity, I directly deprecated the schedule field to encourage CLI users to update their configuration to use these two new fields.

I'm not entirely sure of the logic of the deserialize_raw_configuration function. This why I "request changes". Could you please let me know if the implementation looks fine?

I also updated the tests so the CLI build should be green.

alafanechere · 2022-08-10T10:41:45Z

octavia-cli/octavia_cli/apply/resources.py

+
+        if "schedule_type" in configuration:
+            # If schedule type is manual we do not expect a schedule_data field to be set
+            # TODO: sending a WebConnectionCreate payload without schedule_data (for manual) fails.


@mfsiega-airbyte I tried to send a payload with only schedule_type set to manual but received an error response saying schedule_data must be set. Could you please double-check the behavior of this function and if it matches the expected logic on the API side? I'd be thankful if you could also update the corresponding unit test (test__deserialize_raw_configuration) to ensure all cases are properly handled.

Turns out this was happening when updating from manual -> non-manual; fixed this in the API.

Added a bit of test coverage as well.

Otherwise, LGTM!

timroes · 2022-08-10T12:29:53Z

Looked at the API (not the implementation). Given that oneOf still don't seem to work in our generator (joelittlejohn/jsonschema2pojo#392) this seems like the best solution we can get as an API. If that would work it would be better if we could notate in the type that if scheduleType is cron, the cron expression would be set as well. But this would require oneOf to properly work :( So given this limitation I feel this is the best API we can create for now.

alafanechere

LGTM on the octavia-cli side, thanks!

octavia-cli/unit_tests/test_apply/test_resources.py

Co-authored-by: Augustin <augustin.lafanechere@gmail.com>

michel-tricot · 2022-08-12T01:18:29Z

airbyte-api/src/main/openapi/config.yaml

@@ -4564,6 +4634,10 @@ components:
          $ref: "#/components/schemas/AirbyteCatalog"
        schedule:
          $ref: "#/components/schemas/ConnectionSchedule"
+        scheduleType:


Why is it not encapsulated in one object?

Schedule: type: data:

In my opinion the deeper nesting would make it a bit more unwieldy without too much benefit - encapsulating it under an object doesn't really reduce the cognitive load since every user has to figure out what to do with schedule data anyway whether it's top-level or nested.

I did consider the approach you propose, and I'm not really opposed to it. It comes with the wrinkle that during the migration we have this schedule object that contains both the old and new schemas. (Or - we have some transition period where we have the new schema with some new name, remove the old schema, and then migrate again the new schema to the existing Schedule name.) All in all this felt simpler; but open to feedback here.

No strong opinion on my end. However everything related to migration complexity is a non-issue to me since we are v0. I prefer to have something clean that we want than to make design tradoffs based on backward comp or migrations.

Expose cron scheduling in the Connections APIs

e5ce4c0

github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server area/worker Related to worker labels Aug 3, 2022

mfsiega-airbyte requested review from timroes, a team and terencecho and removed request for a team August 3, 2022 17:32

terencecho approved these changes Aug 3, 2022

View reviewed changes

mfsiega-airbyte and others added 3 commits August 4, 2022 12:30

Update airbyte-api/src/main/openapi/config.yaml

72d8ca1

Co-authored-by: terencecho <terence@airbyte.io>

Update airbyte-server/src/test/java/io/airbyte/server/helpers/Connect…

8b38982

…ionHelpers.java Co-authored-by: terencecho <terence@airbyte.io>

Merge branch 'master' into msiega/cronstrings-api

fdea0e4

mfsiega-airbyte temporarily deployed to more-secrets August 4, 2022 10:35 Inactive

mfsiega-airbyte added 2 commits August 5, 2022 19:03

update octavia-cli tests for new schedule schema, and fix update API …

4218972

…impl

Merge branch 'master' into msiega/cronstrings-api

bca287b

mfsiega-airbyte temporarily deployed to more-secrets August 5, 2022 17:09 Inactive

mfsiega-airbyte temporarily deployed to more-secrets August 5, 2022 17:43 Inactive

terencecho reviewed Aug 5, 2022

View reviewed changes

mfsiega-airbyte temporarily deployed to more-secrets August 8, 2022 10:00 Inactive

mfsiega-airbyte added 2 commits August 8, 2022 13:18

Merge branch 'master' into msiega/cronstrings-api

1266d36

check for null schedule data before updating

f351d9f

mfsiega-airbyte temporarily deployed to more-secrets August 8, 2022 12:03 Inactive

mfsiega-airbyte temporarily deployed to more-secrets August 9, 2022 09:11 Inactive

mfsiega-airbyte temporarily deployed to more-secrets August 9, 2022 09:40 Inactive

mfsiega-airbyte requested a review from alafanechere August 9, 2022 10:20

handle new schedule related fields in generate / apply / import

fe8ed8e

alafanechere temporarily deployed to more-secrets August 10, 2022 10:37 Inactive

update octavia-cli changelog

68ed28a

alafanechere temporarily deployed to more-secrets August 10, 2022 10:40 Inactive

alafanechere requested changes Aug 10, 2022

View reviewed changes

ensure that legacy manual schedule flag is consistent with schedule_type

59455e2

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 07:18 Inactive

update octavia cli test coverage for new schedule schema

3274c0c

mfsiega-airbyte requested a review from alafanechere August 11, 2022 07:42

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 07:42 Inactive

fix failing octavia cli integration tests

9d0833a

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 08:58 Inactive

fix file diff check

3a804e8

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 09:23 Inactive

Merge branch 'master' into msiega/cronstrings-api

09b59e3

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 10:59 Inactive

alafanechere approved these changes Aug 11, 2022

View reviewed changes

octavia-cli/unit_tests/test_apply/test_resources.py Outdated Show resolved Hide resolved

Update octavia-cli/unit_tests/test_apply/test_resources.py

4f8c78a

Co-authored-by: Augustin <augustin.lafanechere@gmail.com>

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 13:39 Inactive

mfsiega-airbyte temporarily deployed to more-secrets August 11, 2022 14:19 Inactive

mfsiega-airbyte merged commit 294ee8f into master Aug 11, 2022

mfsiega-airbyte deleted the msiega/cronstrings-api branch August 11, 2022 17:27

michel-tricot reviewed Aug 12, 2022

View reviewed changes

octavia-squidington-iii mentioned this pull request Aug 19, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1-alpha #15775

Closed

evantahler mentioned this pull request Aug 22, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1 #15857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose cron scheduling in the Connections APIs #15253

Expose cron scheduling in the Connections APIs #15253

mfsiega-airbyte commented Aug 3, 2022

terencecho left a comment

terencecho Aug 5, 2022

mfsiega-airbyte Aug 9, 2022

mfsiega-airbyte commented Aug 9, 2022 •

edited

Loading

alafanechere left a comment

alafanechere Aug 10, 2022

mfsiega-airbyte Aug 11, 2022 •

edited

Loading

timroes commented Aug 10, 2022

alafanechere left a comment

michel-tricot Aug 12, 2022

mfsiega-airbyte Aug 12, 2022

michel-tricot Aug 15, 2022

Expose cron scheduling in the Connections APIs #15253

Expose cron scheduling in the Connections APIs #15253

Conversation

mfsiega-airbyte commented Aug 3, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Tests

terencecho left a comment

Choose a reason for hiding this comment

terencecho Aug 5, 2022

Choose a reason for hiding this comment

mfsiega-airbyte Aug 9, 2022

Choose a reason for hiding this comment

mfsiega-airbyte commented Aug 9, 2022 • edited Loading

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere Aug 10, 2022

Choose a reason for hiding this comment

mfsiega-airbyte Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

timroes commented Aug 10, 2022

alafanechere left a comment

Choose a reason for hiding this comment

michel-tricot Aug 12, 2022

Choose a reason for hiding this comment

mfsiega-airbyte Aug 12, 2022

Choose a reason for hiding this comment

michel-tricot Aug 15, 2022

Choose a reason for hiding this comment

mfsiega-airbyte commented Aug 9, 2022 •

edited

Loading

mfsiega-airbyte Aug 11, 2022 •

edited

Loading