feat(api): Add front50-api package and move PipelineValidator into it #1035

mochacat · 2021-06-04T11:40:08Z

There isn't currently a front50-api package so I created one for the purpose of making PipelineValidator an extension point for plugin developers to utilize.

In the process of migrating this interface and limiting dependencies, I changed the Pipeline controller logic to catch a ValidationException thrown by the validator, instead of having the pipeline validator add errors to the springframework GenericValidationErrors errors. That way the front50-api doesn't need to pull in springframework just for Errors. I thought about implementing a new Errors class but it seemed like I would need to pull in even more dependencies to mimic the functionality of GenericValidationErrors.
I don't see any public implementations of the pipeline validator, but if there are any private implementations they would need to changed to throw the ValidationException instead of adding to Errors.

Pipeline and Timestamped models were also moved over from front50-core in order to limit the dependencies pulled into front50-api. I added mixin classes to front50-core so their original jackson json annotations would be respected. Pipeline was also refactored to not extend a HashMap and use explicit setters/getters.

… json annotations

ajordens · 2021-06-04T21:12:03Z

front50-api/src/main/java/com/netflix/spinnaker/front50/api/model/pipeline/Pipeline.java

+
+import java.util.*;
+
+public class Pipeline implements Timestamped {


Might be able to get away with Lombok in here? Pretty sure it's been used in other -api artifacts.

Rather than @Getter and @Setter, could probably have just added a @Data to the class?

ajordens · 2021-06-04T21:43:45Z

front50-core/src/main/java/com/netflix/spinnaker/front50/jackson/mixins/PipelineMixins.java

+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import java.util.Map;
+
+@JsonIgnoreProperties(


Might be missing something, why are these are @JsonIgnore'd?

If you look at the deleted Pipeline file in front50-core you will see the JsonIgnore for these properties: https://github.com/spinnaker/front50/pull/1035/files/ef8c0e5ee201defe49c325902162cfdc52e4320e#r647609879

Hmm ... interesting. I wonder if they were still being serialized because of the backing map.

Either way, assuming they're still being serialized it seems alright to me.

ajordens · 2021-06-04T21:44:01Z

front50-core/src/main/java/com/netflix/spinnaker/front50/jackson/mixins/PipelineMixins.java

@@ -0,0 +1,53 @@
+/*
+ * Copyright 2021 Netflix, Inc.


Might be worth updating your copyright, no need for it to be Netflix.

ajordens · 2021-06-04T21:52:56Z

front50-api/src/main/java/com/netflix/spinnaker/front50/api/model/pipeline/ForwardingMap.java

+import java.util.Map;
+import java.util.Set;
+
+abstract class ForwardingMap<K, V> implements Map<K, V> {


Wonder if this could be eliminated with a Lombok @Delegate.

I'm not familiar with @Delegate but gave it a quick go without success :(

ajordens · 2021-06-04T21:55:56Z

front50-api/src/main/java/com/netflix/spinnaker/front50/api/model/pipeline/Pipeline.java

+  private List<Trigger> triggers = new ArrayList<Trigger>();
+  private Integer index;
+
+  private List<String> inherit;


Not sure where some of these attributes are coming from? I'm not sure I'd call many of them out as first-class attributes of a pipeline.

ie. runAsUser and serviceAccount don't make sense to me as top-level, am I missing something?

I think you are right. They only belong in triggers, correct? I saw some tests with them as top-level properties, so I ended up removing them from the pipeline objects those tests.

ajordens · 2021-06-04T21:57:04Z

front50-api/src/main/java/com/netflix/spinnaker/front50/api/model/pipeline/Trigger.java

  public String getType() {
    return (String) backing.get("type");
  }

-  public void setType(String type) {
+  public void setType(Object type) {


Why did we lose the type on this? (String -> Object)

ajordens · 2021-06-04T22:15:17Z

...s/src/main/java/com/netflix/spinnaker/front50/migrations/MoveSubpathToLocationMigration.java

@@ -56,7 +56,7 @@ public void run() {

    Predicate<Pipeline> hasExpectedGitRepoArtifact =
        p -> {
-          List<Map> expectedArtifacts = (List<Map>) p.get("expectedArtifacts");
+          List<Map<String, Object>> expectedArtifacts = p.getExpectedArtifacts();


This migration is also seemingly past-due ... I don't think it's a good enough reason to first class expectedArtifacts.

ajordens · 2021-06-04T22:16:18Z

.../src/main/java/com/netflix/spinnaker/front50/migrations/RunAsUserToPermissionsMigration.java

@@ -87,7 +86,7 @@ private void migrate(Pipeline pipeline, Map<String, ServiceAccount> serviceAccou
        value("pipelineId", pipeline.getId()));

    Set<String> newRoles = new HashSet<>();
-    List<String> existingRoles = (List) pipeline.get(ROLES);
+    List<String> existingRoles = pipeline.getRoles();


Ditto here as far as first classing pipeline attributes.

I saw that SharedManagedServiceAccountsMigration needs roles so I left this pipeline attribute in.

ajordens · 2021-06-04T22:17:15Z

...tions/src/main/java/com/netflix/spinnaker/front50/migrations/TriggerConstraintMigration.java

@@ -54,7 +54,7 @@ public void run() {
    log.info("Starting trigger constraint migration");


This is also a migration that could potentially be removed.

ajordens · 2021-06-04T22:17:43Z

...va/com/netflix/spinnaker/front50/migrations/V2PipelineTemplateIncludeToExcludeMigration.java

-          String schema = (String) p.getOrDefault("schema", "");
-          List<String> inherit = (List<String>) p.get("inherit");
-          List<String> exclude = (List<String>) p.get("exclude");
+          String schema = p.getSchema();


Ditto on being able to remove this migration and not first classing these attributes.

ajordens · 2021-06-04T22:22:50Z

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/PipelineController.java

@@ -267,7 +266,11 @@ private void validatePipeline(final Pipeline pipeline, Boolean staleCheck) {
        pipeline.getApplication(), pipeline.getName().trim(), pipeline.getId());

    final GenericValidationErrors errors = new GenericValidationErrors(pipeline);
-    pipelineValidators.forEach(it -> it.validate(pipeline, errors));
+    try {
+      pipelineValidators.forEach(it -> it.validate(pipeline));


Slight difference in behavior wherein previously I think all validators would have run and a collection of errors would have been returned ... vs just the first failure now.

Good point. I ended up creating a ValidatorErrors class to store multiple errors instead of only catching the first one.

ajordens · 2021-06-04T22:24:27Z

Few comments but directionally I think this is 👍

Might be worth annotating some of the new bits in front50-api with a @Beta.

mochacat · 2021-06-08T16:32:48Z

front50-core/src/main/java/com/netflix/spinnaker/front50/model/pipeline/Pipeline.java

-    return (String) super.get("application");
-  }
-
-  @JsonIgnore


@ajordens leaving a comment so you can view the @JsonIgnore annotations in the original Pipeline model in front50-core.

mochacat · 2021-06-08T16:41:34Z

@ajordens made some changes based on your comments. I switched out the ValidationException logic with for the pipeline validator with a custom validator class that can collect multiple errors.

ajordens

This LGTM!

Thanks for the follow-up, great work.

ajordens · 2021-06-10T00:15:24Z

front50-api/src/main/java/com/netflix/spinnaker/front50/api/model/pipeline/Pipeline.java

+
+import java.util.*;
+
+public class Pipeline implements Timestamped {


Rather than @Getter and @Setter, could probably have just added a @Data to the class?

ajordens · 2021-06-10T00:16:35Z

front50-core/src/main/java/com/netflix/spinnaker/front50/jackson/mixins/PipelineMixins.java

+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import java.util.Map;
+
+@JsonIgnoreProperties(


Hmm ... interesting. I wonder if they were still being serialized because of the backing map.

Either way, assuming they're still being serialized it seems alright to me.

…setters

…spinnaker#1035) * feat(api): add api package and move pipeline validator * feat(api): move Trigger and Pipeline to api and use mixin classes for json annotations * chore(migrations): remove out of date migrations * refactor(api): Use lombok to generate getters/setters for Pipeline model * feat(api): use api validator errors class for pipeline validators * fix(api): forwarding map formatting * fix(api): spotless apply fix * fix(api): update sql storage service tests to use new pipleine model setters Co-authored-by: Adam Jordens <adam@jordens.org>

The comment in https://github.com/spinnaker/front50/pull/987/files#r515405296 is no longer true after spinnaker#1035. Pipeline.getTriggers no longer makes a copy, so there's no need to get/modify/set.

…oller.save (#1452) * refactor(web/test): construct Pipeline objects in the first place instead of making Maps and casting * fix(web): restore check for regenerateCronTriggerIds in PipelineController.save https://github.com/spinnaker/front50/pull/1035/files#diff-9b514be177faf5444c86a88ab6bb9e6a0add032bfa67862bc8e33b17c4bb9cc9L159 removed it, but orca's SavePipelineTask still sets it. * refactor(web): adjust pipeline triggers directly The comment in https://github.com/spinnaker/front50/pull/987/files#r515405296 is no longer true after #1035. Pipeline.getTriggers no longer makes a copy, so there's no need to get/modify/set. --------- Co-authored-by: Jason <mcintoshj@gmail.com>

…ines at once to sql db. This is part of: spinnaker/spinnaker#6147. Enhanced PipelineController.java to Added new rest api method bulksave(List<Map> pipelineList, Boolean staleCheck) This method accepts a list of pipelines json. This method checks each and every pipeline for authorization. If the pipeline has valid authorization, then it will be saved to a savedPipelineList. If the pipeline do not have a valid authorization, then it will be saved to a errorPipelineList and an error message is also populated. All the savedPipelinesList will be saved to sql database using bulk insert. This method returns a Map object having the below data: { “Successful”: <count>, “Failed”: <cound>, “Failed_list”: [<array of failed pipelines - (application, pipeline name, etc) and the error message] } Enhanced AuthorizationSupport.java to Added code to accept the pipeline list and authorize the pipelines. Enhanced PipelineControllerSpec.java to Added bean of FiatPermissionEvalutor.java as it is used in the PipelineController constructor. Conflicts: front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/PipelineController.java front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerSpec.groovy front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy due to additional imports from spinnaker#1251 and spinnaker#1252 and org.springframework.context.support.DefaultMessageSourceResolvable has been removed from master by spinnaker#1035 note, this fails to compile because bulkSave has code that expects Pipelines to behave as Maps. As of spinnaker#1035 that's no longer true.

mochacat added 2 commits April 28, 2021 05:16

feat(api): add api package and move pipeline validator

e6ca27a

feat(api): move Trigger and Pipeline to api and use mixin classes for…

ef8c0e5

… json annotations

ajordens reviewed Jun 4, 2021

View reviewed changes

mochacat added 3 commits June 8, 2021 11:07

chore(migrations): remove out of date migrations

f3b5610

refactor(api): Use lombok to generate getters/setters for Pipeline model

717c243

feat(api): use api validator errors class for pipeline validators

b0cc5d3

mochacat commented Jun 8, 2021

View reviewed changes

mochacat requested a review from ajordens June 8, 2021 17:48

ajordens reviewed Jun 10, 2021

View reviewed changes

Merge branch 'master' into api-pipeline-validator

370864e

ajordens added the ready to merge Approved and ready for merge label Jun 10, 2021

mochacat added 3 commits June 10, 2021 06:14

fix(api): forwarding map formatting

f74342c

fix(api): spotless apply fix

2cfb3fe

fix(api): update sql storage service tests to use new pipleine model …

e8eeee9

…setters

mochacat requested a review from ajordens June 11, 2021 19:29

dogonthehorizon approved these changes Jun 11, 2021

View reviewed changes

mergify bot merged commit 4751d87 into spinnaker:master Jun 11, 2021

mergify bot added the auto merged label Jun 11, 2021

spinnakerbot added the target-release/1.27 label Jun 11, 2021

neil-yechenwei mentioned this pull request Jun 29, 2021

front50 is running with error spinnaker/spinnaker#6468

Closed

mochacat mentioned this pull request Jul 9, 2021

fix(storage): Fix deserialization regression for storage services #1041

Merged

This was referenced Jul 16, 2021

fix(api): remove JsonIgnore annotations to fix Pipeline deserialization regression #1042

Merged

fix(api): fix regression for ignoring specific pipeline fields with null values #1043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Add front50-api package and move PipelineValidator into it #1035

feat(api): Add front50-api package and move PipelineValidator into it #1035

mochacat commented Jun 4, 2021

ajordens Jun 4, 2021

ajordens Jun 10, 2021

ajordens Jun 4, 2021

mochacat Jun 8, 2021

ajordens Jun 10, 2021

ajordens Jun 4, 2021

ajordens Jun 4, 2021

mochacat Jun 8, 2021

ajordens Jun 4, 2021

mochacat Jun 8, 2021

ajordens Jun 4, 2021

ajordens Jun 4, 2021

ajordens Jun 4, 2021

mochacat Jun 8, 2021

ajordens Jun 4, 2021

ajordens Jun 4, 2021

ajordens Jun 4, 2021

mochacat Jun 8, 2021

ajordens commented Jun 4, 2021 •

edited

Loading

mochacat Jun 8, 2021

mochacat commented Jun 8, 2021

ajordens left a comment •

edited

Loading

ajordens Jun 10, 2021

ajordens Jun 10, 2021


		import java.util.*;

		public class Pipeline implements Timestamped {

		@@ -54,7 +54,7 @@ public void run() {
		log.info("Starting trigger constraint migration");

feat(api): Add front50-api package and move PipelineValidator into it #1035

feat(api): Add front50-api package and move PipelineValidator into it #1035

Conversation

mochacat commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajordens commented Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

mochacat commented Jun 8, 2021

ajordens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajordens commented Jun 4, 2021 •

edited

Loading

ajordens left a comment •

edited

Loading