From b42c5d793a9beaf710cfbdd1fc0ad4cc82a39918 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Thu, 6 Apr 2017 14:14:22 -0700 Subject: [PATCH] Fix PT-140783423 'branch' only required if used in 'put' step. --- .../ide/vscode/commons/yaml/ast/NodeUtil.java | 12 ++++ .../ide/vscode/concourse/ConcourseModel.java | 61 ++++++++++++++++++- .../vscode/concourse/PipelineYmlSchema.java | 53 ++++++++++++---- .../main/resources/desc/GitSource/branch.md | 4 +- .../vscode/concourse/ConcourseEditorTest.java | 61 +++++++++++++++---- 5 files changed, 165 insertions(+), 26 deletions(-) diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java index 16cea3fd0..ac7b8be72 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java @@ -113,4 +113,16 @@ public class NodeUtil { return null; } + public static Node getProperty(Node node, String propName) { + if (node instanceof MappingNode) { + for (NodeTuple entry : ((MappingNode)node).getValue()) { + String key = NodeUtil.asScalar(entry.getKeyNode()); + if (propName.equals(key)) { + return entry.getValueNode(); + } + } + } + return null; + } + } diff --git a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java index cd3f395a8..454fd92d1 100644 --- a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java +++ b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java @@ -28,12 +28,14 @@ import org.springframework.ide.vscode.commons.yaml.ast.YamlParser; import org.springframework.ide.vscode.commons.yaml.path.ASTRootCursor; import org.springframework.ide.vscode.commons.yaml.path.NodeCursor; import org.springframework.ide.vscode.commons.yaml.path.YamlPath; +import org.springframework.ide.vscode.commons.yaml.path.YamlPathSegment; import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory; import org.springframework.ide.vscode.commons.yaml.schema.YValueHint; import org.springframework.ide.vscode.concourse.util.CollectorUtil; import org.springframework.ide.vscode.concourse.util.StaleFallbackCache; import org.yaml.snakeyaml.Yaml; import org.yaml.snakeyaml.error.YAMLException; +import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; import com.google.common.collect.ImmutableMultiset; @@ -46,6 +48,50 @@ import com.google.common.collect.Multiset; * and completion engine). */ public class ConcourseModel { + + /** + * Wraps around a Node in the AST that represents a 'step' and + * provides methods for accessing information from the node. + */ + public static class StepModel { + + private final String stepType; + private final MappingNode step; + + public StepModel(String stepType, MappingNode step) { + this.stepType = stepType; + this.step = step; + } + + public Node getResourceNameNode() { + Node node = NodeUtil.getProperty(step, "resource"); + return node!=null ? node : NodeUtil.getProperty(step, stepType); + } + + public String getResourceName() { + return NodeUtil.asScalar(getResourceNameNode()); + } + } + + public static class ResourceModel { + + private final Node resource; + + public ResourceModel(Node resource) { + this.resource = resource; + } + + public String getType() { + return NodeUtil.getScalarProperty(resource, "type"); + } + + public boolean hasSourceProperty(String propName) { + YamlPath path = new YamlPath(YamlPathSegment.valueAt("source"), YamlPathSegment.keyAt(propName)); + return path.traverseAmbiguously(resource).findFirst().isPresent(); + } + + } + public static final YamlPath JOB_NAMES_PATH = new YamlPath( anyChild(), valueAt("jobs"), @@ -113,16 +159,25 @@ public class ConcourseModel { * was never successfully parsed. */ public String getResourceType(IDocument doc, String resourceName) { + ResourceModel resource = getResource(doc, resourceName); + if (resource!=null) { + return resource.getType(); + } + return null; + } + + public ResourceModel getResource(IDocument doc, String resourceName) { return getFromAst(doc, (ast) -> { Node resource = RESOURCES_PATH.traverseAmbiguously(new ASTRootCursor(ast)) .map((cursor) -> ((NodeCursor)cursor).getNode()) .filter((resourceNode) -> resourceName.equals(NodeUtil.getScalarProperty(resourceNode, "name"))) .findFirst().orElse(null); if (resource!=null) { - return NodeUtil.getScalarProperty(resource, "type"); + return new ResourceModel(resource); } return null; }); + } /** @@ -220,4 +275,8 @@ public class ConcourseModel { return astTypes; } + public StepModel newStep(String stepType, MappingNode stepNode) { + return new StepModel(stepType, stepNode); + } + } diff --git a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java index bc3a5f3d2..451b34b98 100644 --- a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java +++ b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java @@ -15,16 +15,19 @@ import java.util.Set; import java.util.stream.Collectors; import org.springframework.ide.vscode.commons.languageserver.LanguageIds; +import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector; import org.springframework.ide.vscode.commons.util.MimeTypes; import org.springframework.ide.vscode.commons.util.Renderable; import org.springframework.ide.vscode.commons.util.Renderables; import org.springframework.ide.vscode.commons.util.ValueParseException; import org.springframework.ide.vscode.commons.util.ValueParser; import org.springframework.ide.vscode.commons.util.ValueParsers; +import org.springframework.ide.vscode.commons.util.text.IDocument; import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil; import org.springframework.ide.vscode.commons.yaml.ast.YamlFileAST; import org.springframework.ide.vscode.commons.yaml.path.YamlPath; import org.springframework.ide.vscode.commons.yaml.path.YamlPathSegment; +import org.springframework.ide.vscode.commons.yaml.reconcile.YamlSchemaProblems; import org.springframework.ide.vscode.commons.yaml.schema.BasicYValueHint; import org.springframework.ide.vscode.commons.yaml.schema.DynamicSchemaContext; import org.springframework.ide.vscode.commons.yaml.schema.YType; @@ -39,6 +42,9 @@ import org.springframework.ide.vscode.commons.yaml.schema.YTypedProperty; import org.springframework.ide.vscode.commons.yaml.schema.YValueHint; import org.springframework.ide.vscode.commons.yaml.schema.YamlSchema; import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraints; +import org.springframework.ide.vscode.concourse.ConcourseModel.ResourceModel; +import org.springframework.ide.vscode.concourse.ConcourseModel.StepModel; +import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; import reactor.core.publisher.Flux; @@ -200,14 +206,14 @@ public class PipelineYmlSchema implements YamlSchema { resourceTypes.getSourceType(getResourceTypeTag(models, dc)) ); - AbstractType resource = f.ybean("Resource"); - addProp(resource, "name", resourceNameDef).isRequired(true); - addProp(resource, "type", t_resource_type_name).isRequired(true); - addProp(resource, "source", resourceSource); - addProp(resource, "check_every", t_duration); + AbstractType t_resource = f.ybean("Resource"); + addProp(t_resource, "name", resourceNameDef).isRequired(true); + addProp(t_resource, "type", t_resource_type_name).isRequired(true); + addProp(t_resource, "source", resourceSource); + addProp(t_resource, "check_every", t_duration); AbstractType t_image_resource = f.ybean("ImageResource"); - for (YTypedProperty p : resource.getProperties()) { + for (YTypedProperty p : t_resource.getProperties()) { if (!"name".equals(p.getName())) { t_image_resource.addProperty(p); } @@ -285,6 +291,21 @@ public class PipelineYmlSchema implements YamlSchema { addProp(putStep, "get_params", f.contextAware("GetParams", (dc) -> resourceTypes.getInParamsType(getResourceType("put", models, dc)) )); + putStep.require((dc) -> (IDocument doc, Node parent, MappingNode map, YType type, Set foundProps, IProblemCollector problems) -> { + StepModel step = models.newStep("put", map); + String resourceName = step.getResourceName(); + if (resourceName!=null) { + ResourceModel resource = models.getResource(doc, resourceName); + if (resource!=null) { + if ("git".equals(resource.getType()) && !resource.hasSourceProperty("branch")) { + problems.accept(YamlSchemaProblems.schemaProblem( + "Resource of type 'git' is used in a 'put' step, so it should define 'branch' attribute in its 'source', but it doesn't.", + step.getResourceNameNode() + )); + } + } + } + }); YBeanType taskStep = f.ybean("TaskStep"); addProp(taskStep, "task", t_ne_string); addProp(taskStep, "file", t_string); @@ -343,7 +364,7 @@ public class PipelineYmlSchema implements YamlSchema { addProp(group, "resources", f.yseq(t_resource_name)); addProp(group, "jobs", f.yseq(t_job_name)); - addProp(TOPLEVEL_TYPE, "resources", f.yseq(resource)); + addProp(TOPLEVEL_TYPE, "resources", f.yseq(t_resource)); addProp(TOPLEVEL_TYPE, "jobs", f.yseq(job)); addProp(TOPLEVEL_TYPE, "resource_types", f.yseq(resourceType)); addProp(TOPLEVEL_TYPE, "groups", f.yseq(group)); @@ -360,7 +381,7 @@ public class PipelineYmlSchema implements YamlSchema { { AbstractType source = f.ybean("GitSource"); addProp(source, "uri", t_string).isRequired(true); - addProp(source, "branch", t_string).isRequired(true); + addProp(source, "branch", t_string); //It's more complicated than that! Its only required in 'put' step. So we'll check this as a contrain in put steps! addProp(source, "private_key", t_ne_string); addProp(source, "username", t_ne_string); addProp(source, "password", t_string); @@ -581,11 +602,21 @@ public class PipelineYmlSchema implements YamlSchema { return driver!=null ? driver : "s3"; } - private String getResourceType(String resourceNameProp, ConcourseModel models, DynamicSchemaContext dc) { - String resourceName = getParentPropertyValue("resource", models, dc); + private Node getResourceNameNode(String resourceNameProp, DynamicSchemaContext dc) { + Node resourceName = getParentPropertyNode("resource", models, dc); if (resourceName==null) { - resourceName = getParentPropertyValue(resourceNameProp, models, dc); + resourceName = getParentPropertyNode(resourceNameProp, models, dc); } + return resourceName; + } + + private String getResourceName(String resourceNameProp, DynamicSchemaContext dc) { + Node resourceName = getResourceNameNode(resourceNameProp, dc); + return NodeUtil.asScalar(resourceName); + } + + private String getResourceType(String resourceNameProp, ConcourseModel models, DynamicSchemaContext dc) { + String resourceName = getResourceName(resourceNameProp, dc); if (resourceName!=null) { return models.getResourceType(dc.getDocument(), resourceName); } diff --git a/vscode-extensions/vscode-concourse/src/main/resources/desc/GitSource/branch.md b/vscode-extensions/vscode-concourse/src/main/resources/desc/GitSource/branch.md index 1fb447919..5725f0537 100644 --- a/vscode-extensions/vscode-concourse/src/main/resources/desc/GitSource/branch.md +++ b/vscode-extensions/vscode-concourse/src/main/resources/desc/GitSource/branch.md @@ -1 +1,3 @@ -*Required.* The branch to track. \ No newline at end of file +The branch to track. This is *optional* if the resource is +only used in `get` steps (default value in this case is `master`). +However, it is *required* when used in a `put` step. \ No newline at end of file diff --git a/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java b/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java index 52f142f5e..c3885ae1f 100644 --- a/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java +++ b/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java @@ -21,7 +21,6 @@ import java.util.stream.Collectors; import org.eclipse.lsp4j.Diagnostic; import org.eclipse.lsp4j.DiagnosticSeverity; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.springframework.ide.vscode.commons.languageserver.LanguageIds; import org.springframework.ide.vscode.commons.util.IOUtil; @@ -709,7 +708,10 @@ public class ConcourseEditorTest { " - build\n" ); - editor.assertProblems("not-a-job|does not exist"); + editor.assertProblems( + "build-artefact|should define 'branch'", + "not-a-job|does not exist" + ); } @Test @@ -726,7 +728,7 @@ public class ConcourseEditorTest { " - get: git-repo\n" + " - task: run-build\n" + " file: tasks/some-task.yml\n" + - " - put: build-artefact\n" + + " - put: build-artefact # <- bad\n" + "- name: test\n" + " plan:\n" + " - get: git-repo\n" + @@ -737,6 +739,7 @@ public class ConcourseEditorTest { ); editor.assertProblems( + "build-artefact^ # <- bad|should define 'branch'", "bogus-job|does not exist", "not-a-resource|does not exist" ); @@ -1117,6 +1120,9 @@ public class ConcourseEditorTest { "resources:\n" + "- name: my-git\n" + " type: git\n" + + " source:\n" + + " uri: some-uri\n" + + " branch: master\n" + "jobs:\n" + "- name: do-stuff\n" + " plan:\n" + @@ -1129,6 +1135,9 @@ public class ConcourseEditorTest { "resources:\n" + "- name: my-git\n" + " type: git\n" + + " source:\n" + + " uri: some-uri\n" + + " branch: master\n" + "jobs:\n" + "- name: do-stuff\n" + " plan:\n" + @@ -1313,16 +1322,6 @@ public class ConcourseEditorTest { ); editor.assertProblems("source|'uri' is required"); - //addProp(gitSource, "branch", t_string).isRequired(true); - editor = harness.newEditor( - "resources:\n" + - "- name: foo\n" + - " type: git\n" + - " source:\n" + - " uri: https://yada" - ); - editor.assertProblems("source|'branch' is required"); - //addProp(group, "name", t_ne_string).isRequired(true); editor = harness.newEditor( "groups:\n" + @@ -1331,6 +1330,42 @@ public class ConcourseEditorTest { editor.assertProblems("-^ jobs: []|'name' is required"); } + @Test public void gitBranchRequiredInPutStep() throws Exception { + Editor editor; + + editor = harness.newEditor( + "resources:\n" + + "- name: repo\n" + + " type: git\n" + + " source:\n" + + " uri: git@github.com/johny-coder/test-repo\n" + + "jobs:\n" + + "- name: do-stuff\n" + + " plan:\n" + + " - get: repo\n" + + " - put: repo # <- bad\n" + ); + editor.assertProblems( + "repo^ # <- bad|should define 'branch'" + ); + + editor = harness.newEditor( + "resources:\n" + + "- name: repo\n" + + " type: git\n" + + " source:\n" + + " uri: git@github.com/johny-coder/test-repo\n" + + "jobs:\n" + + "- name: do-stuff\n" + + " plan:\n" + + " - get: repo\n" + + " - put: blah\n" + + " resource: repo # <- bad\n" + ); + editor.assertProblems( + "repo^ # <- bad|should define 'branch'" + ); + } @Test public void dockerImageResourceSourceReconcileAndHovers() throws Exception { Editor editor;