Skip to content

runtime-v2: add github exclusive trigger to schema #977

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 9, 2024
Merged

runtime-v2: add github exclusive trigger to schema #977

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
512b544
runtime-v2: add github exclusive trigger to schema
benbroadaway Sep 18, 2024
4eff102
add JsonDeserialize annotation
benbroadaway Sep 19, 2024
fcf35e1
up
benbroadaway Sep 19, 2024
70e01fe
Merge branch 'master' into github-exclusive-schema
ibodrov Sep 19, 2024
9d5922f
Merge branch 'master' into github-exclusive-schema
ibodrov Sep 21, 2024
d50a6aa
Merge branch 'master' into github-exclusive-schema
benbroadaway Sep 28, 2024
5713e93
Merge remote-tracking branch 'oss/master' into github-exclusive-schema
benbroadaway Oct 1, 2024
7c4b7ca
Merge branch 'master' into github-exclusive-schema
ibodrov Oct 4, 2024
fb1755a
refactor without extra interface
benbroadaway Oct 7, 2024
68765b9
Merge branch 'master' into github-exclusive-schema
ibodrov Oct 7, 2024
5d43d2d
Merge branch 'master' into github-exclusive-schema
ibodrov Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions runtime/v2/model/src/main/java/com/walmartlabs/concord/runtime/v2/model/ExclusiveMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,12 @@
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import org.immutables.value.Value;

import java.io.Serial;
import java.io.Serializable;

@Value.Immutable
@Value.Style(jdkOnly = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonSerialize(as = ImmutableExclusiveMode.class)
@JsonDeserialize(as = ImmutableExclusiveMode.class)
@JsonDeserialize(as = ImmutableDefaultExclusiveMode.class)
public interface ExclusiveMode extends Serializable {

long serialVersionUID = 1L;

@Value.Parameter
@JsonProperty(value = "group", required = true)
String group();
Expand Down Expand Up @@ -65,6 +60,19 @@ enum Mode {
}

static ExclusiveMode of(String group, Mode mode) {
return ImmutableExclusiveMode.of(group, mode);
return ImmutableDefaultExclusiveMode.of(group, mode);
}

@Value.Immutable
@Value.Style(jdkOnly = true)
@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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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" ]
        }
      }
    }


@Serial
long serialVersionUID = 1L;

}

}
4 changes: 3 additions & 1 deletion ...el/src/main/java/com/walmartlabs/concord/runtime/v2/model/GithubTriggerExclusiveMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,17 @@
import org.immutables.value.Value;

import javax.annotation.Nullable;
import java.io.Serial;
import java.io.Serializable;

@Value.Immutable
@Value.Style(jdkOnly = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonSerialize(as = ImmutableGithubTriggerExclusiveMode.class)
@JsonDeserialize(as = ImmutableGithubTriggerExclusiveMode.class)
public interface GithubTriggerExclusiveMode extends Serializable {
public interface GithubTriggerExclusiveMode extends ExclusiveMode, Serializable {

@Serial
long serialVersionUID = 1L;

@Nullable
Expand Down
4 changes: 2 additions & 2 deletions ...2/model/src/main/java/com/walmartlabs/concord/runtime/v2/parser/ConfigurationGrammar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ public final class ConfigurationGrammar {

private static final Parser<Atom, ExclusiveMode> exclusive =
betweenTokens(JsonToken.START_OBJECT, JsonToken.END_OBJECT,
with(ImmutableExclusiveMode::builder,
with(ImmutableDefaultExclusiveMode::builder,
o -> options(
mandatory("group", stringNotEmptyVal.map(o::group)),
optional("mode", enumVal(ExclusiveMode.Mode.class).map(o::mode))))
.map(ImmutableExclusiveMode.Builder::build));
.map(ImmutableDefaultExclusiveMode.Builder::build));

public static final Parser<Atom, ExclusiveMode> exclusiveVal =
orError(exclusive, YamlValueType.EXCLUSIVE_MODE);
Expand Down
3 changes: 2 additions & 1 deletion runtime/v2/model/src/main/java/com/walmartlabs/concord/runtime/v2/schema/TriggerMixIn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaInject;
import com.kjetland.jackson.jsonSchema.annotations.JsonSchemaString;
import com.walmartlabs.concord.runtime.v2.model.ExclusiveMode;
import com.walmartlabs.concord.runtime.v2.model.GithubTriggerExclusiveMode;
import com.walmartlabs.concord.runtime.v2.model.Trigger;

import java.util.List;
Expand Down Expand Up @@ -111,7 +112,7 @@ interface GithubTriggerParams extends DefaultTriggerParams {
GithubTriggerConditions conditions();

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

interface GithubTriggerConditions {
@JsonProperty(value = "type", required = true)
Expand Down
4 changes: 2 additions & 2 deletions server/impl/src/main/java/com/walmartlabs/concord/server/process/PayloadUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ public static OffsetDateTime getStartAt(Payload p) {
return null;
}

if (v instanceof String) {
if (v instanceof String iso) {
OffsetDateTime t;
try {
t = DateTimeUtils.fromIsoString((String) v);
t = DateTimeUtils.fromIsoString(iso);
} catch (DateTimeParseException e) {
throw new ProcessException(p.getProcessKey(), "Invalid '" + k + "' format, expected an ISO-8601 value, got: " + v);
}
Expand Down
Loading