Skip to content

Conversation

@benbroadaway
Copy link
Collaborator

groupBy currently isn't represented in the runtime-v2 json schema.

@benbroadaway benbroadaway requested review from a team and ibodrov September 18, 2024 22:24
dankle
dankle previously approved these changes Sep 18, 2024
@benbroadaway benbroadaway added the wip Work in progress, do not merge label Sep 19, 2024
@benbroadaway benbroadaway removed the wip Work in progress, do not merge label Sep 19, 2024
@benbroadaway benbroadaway requested a review from a team September 19, 2024 16:31
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonSerialize(as = ImmutableDefaultExclusiveMode.class)
@JsonDeserialize(as = ImmutableDefaultExclusiveMode.class)
interface DefaultExclusiveMode extends ExclusiveMode {
Copy link
Collaborator

@ibodrov ibodrov Sep 23, 2024

Choose a reason for hiding this comment

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

Not sure why do we need a separate interface here? Does it change the serialization of TriggerMixIn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One @Immutable class should not extends another @Immutable class.

@Value.Immutable
public interface MyInterface {
    boolean isTrue();
}

@Value.Immutable
public interface AnotherInterface extends MyInterface {
    boolean isFalse();
}
AnotherInterface.java: (immutables:subtype) Should not inherit com.example.MyInterface which is
a value type itself. Avoid extending from another abstract value type. Better to share common
abstract class or interface which are not carrying @Immutable annotation. If still extending from
immutable abstract type be ready to face some incoherences in generated types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does it change the serialization of TriggerMixIn?

Is the concern in this question regarding somewhere we deserialize it from a value stored in the db? i.e. a value stored pre-upgrade may fail after this change is in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I misunderstood what was happening here, my bad.

Copy link
Contributor

@brig brig Oct 1, 2024

Choose a reason for hiding this comment

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

I'm still not quite sure why a new class/interface appeared here...

Wouldn't it be enough to just change:

GithubTriggerMixIn:

            @JsonProperty("exclusive")
            ExclusiveMode exclusive();

to

            @JsonProperty("exclusive")
            GithubTriggerExclusiveMode exclusive();

?

Copy link
Contributor

Choose a reason for hiding this comment

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

something like this:

@@ -111,7 +112,7 @@ public interface TriggerMixIn extends Trigger {
             GithubTriggerConditions conditions();
 
             @JsonProperty("exclusive")
-            ExclusiveMode exclusive();
+            GithubTriggerExclusiveMode exclusive();
 
             interface GithubTriggerConditions {
                 @JsonProperty(value = "type", required = true)
@@ -174,6 +175,9 @@ public interface TriggerMixIn extends Trigger {
 
             @JsonProperty("conditions")
             Map<String, Object> conditions();
+
+            @JsonProperty("exclusive")
+            ExclusiveMode exclusive();
         }
     }
 
@@ -187,8 +191,5 @@ public interface TriggerMixIn extends Trigger {
 
         @JsonProperty("arguments")
         Map<String, Object> arguments();
-
-        @JsonProperty("exclusive")
-        ExclusiveMode exclusive();
     }
 }

Copy link
Collaborator

@ibodrov ibodrov Oct 2, 2024

Choose a reason for hiding this comment

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

Not sure if that would be reflected in the resulting schema?
Currently (with the PRs changes), it's

"triggers" : {
      "type" : "array",
      "items" : {
        "oneOf" : [ {
          "$ref" : "#/definitions/GenericTrigger"
        }, {
          "$ref" : "#/definitions/CronTrigger"
        }, {
          "$ref" : "#/definitions/ManualTrigger"
        }, {
          "$ref" : "#/definitions/GithubTrigger"
        }, {
          "$ref" : "#/definitions/OneOpsTrigger"
        } ]
      }
}

With GithubTrigger containing

"GithubTriggerParams" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "version" : {
          "type" : "integer",
          "enum" : [ 2 ]
        },
        "exclusive" : {
          "$ref" : "#/definitions/GithubTriggerExclusiveMode"
        },
        ...

and other trigger types using the base ExclusiveMode:

"ManualTriggerParams" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "name" : {
          "type" : "string"
        },
        "arguments" : {
          "type" : "object"
        },
        "activeProfiles" : {
          "type" : "array",
          "items" : {
            "type" : "string"
          }
        },
        "entryPoint" : {
          "type" : "string"
        },
        "exclusive" : {
          "$ref" : "#/definitions/ExclusiveMode"
        }
      },
      "required" : [ "entryPoint" ]
    },

Which seems fine to me?

Copy link
Contributor

@brig brig Oct 2, 2024

Choose a reason for hiding this comment

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

Not sure if that would be reflected in the resulting schema?

It should be, why not?

from master branch:

...
"GithubTriggerParams" : {
...
        "exclusive" : {
          "$ref" : "#/definitions/ExclusiveMode"
        },
....

with just ExclusiveMode -> GithubTriggerExclusiveMode change:

...
"GithubTriggerParams" : {
...
        "exclusive" : {
          "$ref" : "#/definitions/GithubTriggerExclusiveMode"
        },
....

    "GithubTriggerExclusiveMode" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "group" : {
          "type" : "string"
        },
        "groupBy" : {
          "type" : "string"
        },
        "mode" : {
          "type" : "string",
          "enum" : [ "cancel", "cancelOld", "wait" ]
        }
      }
    }

ibodrov
ibodrov previously approved these changes Oct 1, 2024
@ibodrov
Copy link
Collaborator

ibodrov commented Oct 1, 2024

@brig should we mark GithubTriggerExclusiveMode#group and #groupBy fields as @Deprecated?
(in a separate PR, perhaps)

@ibodrov
Copy link
Collaborator

ibodrov commented Oct 2, 2024

On a separate note, I don't think this is right?

"ManualTrigger" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "manual" : {
          "$ref" : "#/definitions/ManualTriggerParams"
        },
        "exclusive" : {
          "$ref" : "#/definitions/ExclusiveMode"
        }
      },
      "title" : "Manual Trigger"
    },
    "ManualTriggerParams" : {
      "type" : "object",
      "additionalProperties" : false,
      "properties" : {
        "name" : {
          "type" : "string"
        },
        "arguments" : {
          "type" : "object"
        },
        "activeProfiles" : {
          "type" : "array",
          "items" : {
            "type" : "string"
          }
        },
        "entryPoint" : {
          "type" : "string"
        },
        "exclusive" : {
          "$ref" : "#/definitions/ExclusiveMode"
        }
      },
      "required" : [ "entryPoint" ]
    },

Why both ManualTrigger and ManualTriggerParams have exclusive as a property? Doesn't look right to me. 👀

@brig
Copy link
Contributor

brig commented Oct 2, 2024

On a separate note, I don't think this is right?

yep :(

this lines

should be in ManualTriggerParams

@ibodrov ibodrov merged commit 3d8bbe4 into master Oct 9, 2024
4 checks passed
@ibodrov ibodrov deleted the github-exclusive-schema branch October 9, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants