airbyte icon indicating copy to clipboard operation
airbyte copied to clipboard

Published 20 hours ago • verified publisher icon airbytehq

feat: added cron field to connections form

Open harshithmullapudi opened this issue 2 years ago • 5 comments

What

This PR is adding fields related to the cron feature. Issue: https://github.com/airbytehq/airbyte/issues/13061

Screenshot 2022-09-07 at 1 17 46 AM

https://user-images.githubusercontent.com/17528887/189032649-65fff7cd-a1b2-463e-bd0b-3433090f7836.mov

How

We have added Cron as a new frequency type to the dropdown based on which we will show 2 new fields (Cron expression and Timezone).

Recommended reading order

  1. ConnectionForm.tsx
  2. ScheduleField.tsx
  3. formConfig.tsx
  4. ConnectionTable.tsx
  5. FrequencyCell.tsx

🚨 User Impact 🚨

There are no breaking changes. The new feature CRON should be up but should not break the existing manual/basic scheduling functionality.

harshithmullapudi avatar Sep 06 '22 19:09 harshithmullapudi

@lmossman thanks a lot for the review. I made the changes as suggested. Kindly look at them when you get time.

harshithmullapudi avatar Sep 07 '22 05:09 harshithmullapudi

Looks like you've got some merge conflicts!

krishnaglick avatar Sep 07 '22 18:09 krishnaglick

Looks like you've got some merge conflicts!

Yeah small one resolving it.

harshithmullapudi avatar Sep 07 '22 18:09 harshithmullapudi

Not sure why but one of the comments I replied to is getting minimized. Linking it here so that you don't miss it @harshithmullapudi : https://github.com/airbytehq/airbyte/pull/16375#discussion_r965422757

lmossman avatar Sep 08 '22 01:09 lmossman

please merge this, its really important feature

jaroslaw-weber avatar Sep 14 '22 00:09 jaroslaw-weber

@lmossman can I merge this?

harshithmullapudi avatar Sep 14 '22 15:09 harshithmullapudi

@harshithmullapudi I left one more comment about the error message we are using for the invalid cron expression. Feel free to merge after you address that!

lmossman avatar Sep 14 '22 15:09 lmossman

@harshithmullapudi I left one more comment about the error message we are using for the invalid cron expression. Feel free to merge after you address that!

Sorry I already changed it and forgot to push that. Thanks a lot for pointing that out. Thanks a lot for your review and time.

harshithmullapudi avatar Sep 14 '22 15:09 harshithmullapudi

@harshithmullapudi looks like the builds are green so I think we can go ahead and merge

lmossman avatar Sep 14 '22 16:09 lmossman