fix: API returns error when job template has cache setting (#174)

* fix: merge template before cache setting parsing

* feat: add tests

Author: y-oksaku

lib/phase/flatten.js

@@ -730,7 +730,7 @@ function flattenStageSetupAndTeardownJobs(doc) {
  * @param   {TemplateFactory}  templateFactory       Template Factory to get templates
  * @returns {Promise}
  */
-function flattenPhase(parsedDoc, templateFactory) {
+async function flattenPhase(parsedDoc, templateFactory) {
     const doc = parsedDoc;
 
     // Flatten stage setup and teardown into jobs
@@ -745,36 +745,39 @@ function flattenPhase(parsedDoc, templateFactory) {
     // Flatten shared into jobs
     doc.jobs = flattenSharedIntoJobs(parsedDoc.shared, parsedDoc.jobs);
 
+    // Flatten templates
+    const { warnings, newJobs } = await flattenTemplates(doc, templateFactory);
+
+    // Clean through the job values
+    const cleanedJobs = cleanComplexEnvironment(newJobs);
+
+    // Convert steps into proper expanded output
+    const convertedJobs = convertSteps(cleanedJobs);
+
+    // Append flattened jobs and return flattened doc
+    doc.jobs = convertedJobs;
+    delete doc.shared;
+
+    // Flatten cache settings if the pipeline has cache settings
     if (parsedDoc.cache) {
         doc.jobs = flattenCacheSettings(parsedDoc.cache, doc.jobs);
         delete doc.cache;
     } else {
+        // Some job has cache config, but there is no cache settings in the pipeline
         Object.keys(doc.jobs).forEach(jobName => {
             if (doc.jobs[jobName].cache !== undefined) {
                 throw new Error('Cache disable/enable is set without the cache setting itself.');
             }
         });
     }
 
-    // Flatten templates
-    return flattenTemplates(doc, templateFactory).then(({ warnings, newJobs }) => {
-        // Clean through the job values
-        const cleanedJobs = cleanComplexEnvironment(newJobs);
-        // Convert steps into proper expanded output
-        const convertedJobs = convertSteps(cleanedJobs);
+    const errors = checkAdditionalRules(doc);
 
-        // Append flattened jobs and return flattened doc
-        doc.jobs = convertedJobs;
-        delete doc.shared;
-
-        const errors = checkAdditionalRules(doc);
-
-        if (errors) {
-            throw new Error(errors.toString());
-        }
+    if (errors) {
+        throw new Error(errors.toString());
+    }
 
-        return { warnings, flattenedDoc: doc };
-    });
+    return { warnings, flattenedDoc: doc };
 }
 
 module.exports = {

test/data/basic-job-and-template-with-cache.json

@@ -0,0 +1,80 @@
+{
+    "annotations": {},
+    "parameters": {},
+    "jobs": {
+        "enabled": [{
+            "annotations": {},
+            "image": "node:4",
+            "commands": [
+                {
+                    "name": "install",
+                    "command": "npm install"
+                },
+                {
+                    "name": "test",
+                    "command": "npm test"
+                }
+            ],
+            "environment": {
+                "SD_TEMPLATE_FULLNAME": "mytemplate",
+                "SD_TEMPLATE_NAME": "mytemplate",
+                "SD_TEMPLATE_NAMESPACE": "",
+                "SD_TEMPLATE_VERSION": "1.2.3"
+            },
+            "requires": [
+                "~pr",
+                "~commit"
+            ],
+            "cache": {
+                "pipeline": [ "pipeline-cache" ],
+                "event": [],
+                "job": []
+            },
+            "secrets": [],
+            "settings": {},
+            "templateId": 7754
+        }],
+        "disabled": [{
+            "annotations": {},
+            "image": "node:4",
+            "commands": [
+                {
+                    "name": "install",
+                    "command": "npm install"
+                },
+                {
+                    "name": "test",
+                    "command": "npm test"
+                }
+            ],
+            "environment": {
+                "SD_TEMPLATE_FULLNAME": "mytemplate",
+                "SD_TEMPLATE_NAME": "mytemplate",
+                "SD_TEMPLATE_NAMESPACE": "",
+                "SD_TEMPLATE_VERSION": "1.2.3"
+            },
+            "requires": [
+                "~pr",
+                "~commit"
+            ],
+            "secrets": [],
+            "settings": {},
+            "templateId": 7754
+        }]
+    },
+    "workflowGraph": {
+        "nodes": [
+            { "name": "~pr" },
+            { "name": "~commit" },
+            { "name": "enabled" },
+            { "name": "disabled" }
+        ],
+        "edges": [
+            { "src": "~pr", "dest": "enabled" },
+            { "src": "~commit", "dest": "enabled" },
+            { "src": "~pr", "dest": "disabled" },
+            { "src": "~commit", "dest": "disabled" }
+        ]
+    },
+    "subscribe": {}
+}

test/data/basic-job-and-template-with-cache.yaml

@@ -0,0 +1,11 @@
+cache:
+  pipeline: [ pipeline-cache ]
+
+jobs:
+  enabled:
+    requires: [ ~pr, ~commit ]
+    template: TemplateCacheTestNamespace/cachetemplate@1
+  disabled:
+    requires: [ ~pr, ~commit ]
+    template: TemplateCacheTestNamespace/cachetemplate@2
+    cache: false

test/data/templateWithCache.json

@@ -0,0 +1,16 @@
+{
+    "id": 7754,
+    "name": "mytemplate",
+    "version": "1.2.3",
+    "description": "test template",
+    "maintainer": "bar@foo.com",
+    "labels": [],
+    "config": {
+        "image": "node:4",
+        "cache": true,
+        "steps": [
+            { "install": "npm install" },
+            { "test": "npm test" }
+        ]
+    }
+}

test/index.test.js

@@ -355,6 +355,8 @@ describe('config parser', () => {
                 let jobParametersTemplate;
                 let providerTemplate;
                 let jobWithRequiresTemplate;
+                let jobWithCacheTemplate1;
+                let jobWithCacheTemplate2;
 
                 beforeEach(() => {
                     firstTemplate = JSON.parse(loadData('template.json'));
@@ -367,6 +369,8 @@ describe('config parser', () => {
                     jobParametersTemplate = JSON.parse(loadData('templateWithParameters.json'));
                     providerTemplate = JSON.parse(loadData('templateWithProvider.json'));
                     jobWithRequiresTemplate = JSON.parse(loadData('templateWithRequires.json'));
+                    jobWithCacheTemplate1 = JSON.parse(loadData('templateWithCache.json'));
+                    jobWithCacheTemplate2 = JSON.parse(loadData('templateWithCache.json'));
                     templateFactoryMock.getFullNameAndVersion.returns({ isVersion: true });
                     templateFactoryMock.getTemplate.withArgs('mytemplate@1.2.3').resolves(firstTemplate);
                     templateFactoryMock.getTemplate.withArgs('yourtemplate@2').resolves(secondTemplate);
@@ -387,6 +391,14 @@ describe('config parser', () => {
                     templateFactoryMock.getTemplate
                         .withArgs('JobTestNamespace/jobrequirestemplate@2')
                         .resolves(jobWithRequiresTemplate);
+                    // Since the resolved values are the same object, their properties can be overwritten during other parsing.
+                    // We need to separate that resolved values in tests.
+                    templateFactoryMock.getTemplate
+                        .withArgs('TemplateCacheTestNamespace/cachetemplate@1')
+                        .resolves(jobWithCacheTemplate1);
+                    templateFactoryMock.getTemplate
+                        .withArgs('TemplateCacheTestNamespace/cachetemplate@2')
+                        .resolves(jobWithCacheTemplate2);
                 });
 
                 it('flattens templates successfully', () =>
@@ -597,6 +609,15 @@ describe('config parser', () => {
                         );
                     }));
 
+                it('flattens cache config with templates containing parsed cache setting', () =>
+                    parser({
+                        yaml: loadData('basic-job-and-template-with-cache.yaml'),
+                        templateFactory: templateFactoryMock,
+                        triggerFactory
+                    }).then(data => {
+                        assert.deepEqual(data, JSON.parse(loadData('basic-job-and-template-with-cache.json')));
+                    }));
+
                 it('flattens templates with provider', () =>
                     parser({
                         yaml: loadData('basic-job-with-provider.yaml'),