fix(artifacts): Expected Artifacts should be trigger specific

[jervi]

When setting up multiple triggers with different expected artifacts, triggering the pipeline fails if any defined expected artifact is missing, even if it is defined on another trigger. This fix filters the list of expected artifacts to make sure only the artifacts that are relevant for the current trigger is evaluated.

Example:

If you create two triggers like this, with different artifact constraints:

This would result in a pipeline config like this:

{
  "expectedArtifacts": [
    {
      "defaultArtifact": {
        "customKind": true,
        "id": "3c55a698-aa57-4f38-9326-a99511b8e7b0"
      },
      "displayName": "my-artifact",
      "id": "89709fb4-4ad1-4267-ba14-77befa93b58d",
      "matchArtifact": {
        "artifactAccount": "custom-artifact",
        "customKind": true,
        "id": "dfa2e113-eb4f-4d20-adec-db0756ec4f44",
        "name": "s3://my-bucket/myfile",
        "type": "s3/object"
      },
      "useDefaultArtifact": false,
      "usePriorArtifact": false
    },
    {
      "defaultArtifact": {
        "customKind": true,
        "id": "26023c36-8257-4adb-a764-485e5ddb8462"
      },
      "displayName": "another-artifact",
      "id": "5d35f6ed-1336-4bc6-8c46-2c18df238163",
      "matchArtifact": {
        "artifactAccount": "custom-artifact",
        "customKind": true,
        "id": "0ea9e8b6-94e1-4df6-9004-88efade8cf67",
        "type": "custom/object"
      },
      "useDefaultArtifact": false,
      "usePriorArtifact": false
    }
  ],
  "keepWaitingPipelines": false,
  "limitConcurrent": true,
  "schema": "1",
  "spelEvaluator": "v4",
  "stages": [ ... ],
  "triggers": [
    {
      "enabled": true,
      "expectedArtifactIds": [
        "89709fb4-4ad1-4267-ba14-77befa93b58d"
      ],
      "pubsubSystem": "amazon",
      "subscriptionName": "pulse-routing-configurations-dev",
      "type": "pubsub"
    },
    {
      "application": "jervi",
      "enabled": true,
      "expectedArtifactIds": [
        "5d35f6ed-1336-4bc6-8c46-2c18df238163"
      ],
      "pipeline": "cee145ad-21b5-42a8-9894-b34d3f1cfed8",
      "status": [
        "successful"
      ],
      "type": "pipeline"
    }
  ]
}

Before the fix, the defined triggers would not trigger unless all the defined artifacts were present. Here I have tried to start the pipeline using the pipeline trigger, but it won't start because it can't find the artifact that is defined for the pubsub trigger:

As you can see in the original PR for this feature, it actually used to filter the expected artifacts correctly.

[spinnakerbot]

The following commits need their title changed:

• d576f48: bug(artifacts): Expected Artifacts are trigger specific

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

[dbyron-sf]

There's a test a failure...but also, I could use some help making more sense of this. It feels right, but some examples would help. Like, what does pipeline json with multiple triggers look like? From what I can tell of the tests in this PR, I see one with one trigger, and one with none.

[jervi]

Yeah, sorry, I was fixing the tests but had to run to pick up kids. I'll get them fixed ASAP.
I'll add some examples as well (I was already working on it).

[jervi]

More test cases added, and I have updated the PR description with more information.

This last commit also includes a fix that is specific to Pipeline triggers only - this trigger is using another code path than the other triggers (DependentPipelineStarter vs. OperationsController) which didn't set the expectedArtifactIds field at all. The fix will figure out the correct trigger from the triggers array in the pipeline config, and copy it to the trigger field in the execution context.

[dbyron-sf]

This looks good. I'm a little concerned about people who may be depending on the current behavior -- that a pipeline only triggers when all of its expected artifacts are present. What do you think about putting the new behavior behind a feature flag?

[jervi]

To be honest, I think they then depend on a bug and needs to update their pipelines instead. The fix is easy; just add the expected artifact constraints to all triggers.
Since this PR just reverts the Spinnaker behavior to the earlier, correct state, I don't think it should be put behind a feature flag.

[dbyron-sf]

OK, fair enough. Could you add a release note for this at least? https://github.com/spinnaker/spinnaker.io/blob/master/content/en/docs/releases/next-release-preview/index.md

[dbyron-sf]

Thanks for the release note!