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

Merge standard sync schedule to standard sync #3472

Merged
merged 13 commits into from
May 20, 2021

Conversation

tuliren
Copy link
Contributor

@tuliren tuliren commented May 18, 2021

What

How

  • Add the two fields from StandardSyncSchema to StandardSync.
    • manual (required)
    • schedule
  • Add a migration to join StandardSyncSchema onto StandardSync.

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. New schema file: StandardSyncSchedule.yaml.
  2. New migration file: MigrationV0_24_0.java.
  3. The rest.

@tuliren tuliren force-pushed the liren/merge-std-sync-schedule branch from 11dd1e5 to a27e318 Compare May 19, 2021 07:04
@tuliren tuliren marked this pull request as ready for review May 19, 2021 18:28
@tuliren
Copy link
Contributor Author

tuliren commented May 19, 2021

The e2e tests passed locally.

@@ -65,7 +65,7 @@ public MigrationV0_14_1() {

@Override
public String getVersion() {
return "v0.11.1-alpha";
return "v0.14.1-alpha";
Copy link
Contributor

Choose a reason for hiding this comment

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

yikes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a big deal since this is in test.

@tuliren tuliren merged commit 21ba32c into master May 20, 2021
@tuliren tuliren deleted the liren/merge-std-sync-schedule branch May 20, 2021 22:10
# Ideally schedule and manual should be a union, but java
# codegen does not handle the union type properly.
# When schedule is defined, manual should be false.
schedule:
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think something like that would be cleaner like:

schedule:
  type: object
  required:
    - type
  properties:
    type: enum(manual, frequency, other_type)
    frequency:
       type: object   
       properties:
          timeUnit:
             type: string
             enum:
               - minutes
               - hours
               - days
               - weeks
               - months
           units:
             type: integer
    other_type:
       type: object  
       properties:
          // blah 

This way you don't polute StandardSync with manual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally yes. The json-schema does support union with the oneOf keyword. However, this keyword is not supported by the java code generator jsonschema2pojo: joelittlejohn/jsonschema2pojo#392

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's why I had to do what Michel is proposing for the NotificationType and OperatorType fields:

# the jsonschema2pojo does not seem to support it yet: https://github.com/joelittlejohn/jsonschema2pojo/issues/392

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I misunderstood what Michel was proposing. Basically we can make all those enum properties optional fields, and manually make sure only one of them exists on the object. That can work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the enum makes it a valid design (the best would be oneof ofcourse) instead of a hack

@michel-tricot
Copy link
Contributor

Amazing change :) Code is a lot cleaner now! Thanks for updating the different docs along the way

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.

Consolidate StandardSyncSchedule into StandardSync
4 participants