Supporting Multiple Files With the Same Name (#327)

* changed ubuntu runner * changed minikube action * Version formatting * nonedriveR * update kube version * installing conntrack' * updated other actions * update bg ingress api version * prettify * updated ingress backend for new api version * Added path type * prettify * added logging * added try catch logic to prevent future failures if annotations fail since failing annotations shouldn't affect users * added nullcheck * Added fallback filename if workflow fails to get github filepath due to runner issues * cleanup * added oliver's feedback + unit test demonstrating regex glitch and fix * no longer using blank string for failed regex * add tests and dont split so much * testing * file fix * without fix * Revert "without fix" This reverts commit 8da79a8. * fixing labels test * pretty * refactored getting tmp filename to use entire path, and refactored private to use filepath relative to tmp dir * wip * merging master * this should fail * added UTs * restructured plus UTs plus debug logs * resolved dir not existing and UTs * cleanup * no silent failure * Reverting private logic * this might work * root level files for temp... bizarre issue * need to actually write contents * no more cwdir * moving everything out of temp * deleting unused function * supporting windows filepaths for private cluster shallow path generation --------- Co-authored-by: David Gamero <[email protected]>
Azure · Aug 7, 2024 · df58fb4 · df58fb4
1 parent a999ffc
commit df58fb4
Show file tree

Hide file tree

Showing 12 changed files with 205 additions and 114 deletions.
diff --git a/.github/workflows/run-integration-tests-basic.yml b/.github/workflows/run-integration-tests-basic.yml
@@ -64,9 +64,13 @@ jobs:
               images: nginx:1.14.2
               manifests: |
                  test/integration/manifests/test.yml
+                 test/integration/manifests/manifest_test_dir/test.yml
               action: deploy
 
          - name: Checking if deployments and services were created
            run: |
               python test/integration/k8s-deploy-test.py namespace=${{ env.NAMESPACE }} kind=Deployment name=nginx-deployment containerName=nginx:1.14.2 labels=app:nginx,workflow:actions.github.com-k8s-deploy,workflowFriendlyName:Minikube_Integration_Tests_-_basic selectorLabels=app:nginx
               python test/integration/k8s-deploy-test.py namespace=${{ env.NAMESPACE }} kind=Service name=nginx-service labels=workflow:actions.github.com-k8s-deploy,workflowFriendlyName:Minikube_Integration_Tests_-_basic selectorLabels=app:nginx
+
+              python test/integration/k8s-deploy-test.py namespace=${{ env.NAMESPACE }} kind=Deployment name=nginx-deployment3 containerName=nginx:1.14.2 labels=app:nginx3,workflow:actions.github.com-k8s-deploy,workflowFriendlyName:Minikube_Integration_Tests_-_basic selectorLabels=app:nginx3
+              python test/integration/k8s-deploy-test.py namespace=${{ env.NAMESPACE }} kind=Service name=nginx-service3 labels=workflow:actions.github.com-k8s-deploy,workflowFriendlyName:Minikube_Integration_Tests_-_basic selectorLabels=app:nginx3
diff --git a/src/strategyHelpers/deploymentHelper.ts b/src/strategyHelpers/deploymentHelper.ts
@@ -10,12 +10,7 @@ import {Kubectl, Resource} from '../types/kubectl'
 import {deployPodCanary} from './canary/podCanaryHelper'
 import {deploySMICanary} from './canary/smiCanaryHelper'
 import {DeploymentConfig} from '../types/deploymentConfig'
-import {
-   deployBlueGreen,
-   deployBlueGreenIngress,
-   deployBlueGreenService
-} from './blueGreen/deploy'
-import {deployBlueGreenSMI} from './blueGreen/deploy'
+import {deployBlueGreen} from './blueGreen/deploy'
 import {DeploymentStrategy} from '../types/deploymentStrategy'
 import * as core from '@actions/core'
 import {
@@ -39,7 +34,6 @@ import {
    normalizeWorkflowStrLabel
 } from '../utilities/githubUtils'
 import {getDeploymentConfig} from '../utilities/dockerUtils'
-import {deploy} from '../actions/deploy'
 import {DeployResult} from '../types/deployResult'
 
 export async function deployManifests(

diff --git a/src/types/privatekubectl.test.ts b/src/types/privatekubectl.test.ts
@@ -1,12 +1,14 @@
+import * as fileUtils from '../utilities/fileUtils'
+import * as fs from 'fs'
 import {
    PrivateKubectl,
    extractFileNames,
-   replaceFileNamesWithBaseNames
+   replaceFileNamesWithShallowNamesRelativeToTemp
 } from './privatekubectl'
 import * as exec from '@actions/exec'
 
 describe('Private kubectl', () => {
-   const testString = `kubectl annotate -f testdir/test.yml,test2.yml,testdir/subdir/test3.yml -f test4.yml --filename test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/manifests/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
+   const testString = `kubectl annotate -f /tmp/testdir/test.yml,/tmp/test2.yml,/tmp/testdir/subdir/test3.yml -f /tmp/test4.yml --filename /tmp/test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
    const mockKube = new PrivateKubectl(
       'kubectlPath',
       'namespace',
@@ -15,19 +17,32 @@ describe('Private kubectl', () => {
       'resourceName'
    )
 
+   const spy = jest
+      .spyOn(fileUtils, 'getTempDirectory')
+      .mockImplementation(() => {
+         return '/tmp'
+      })
+
+   jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {})
+   jest.spyOn(fs, 'readFileSync').mockImplementation((filename) => {
+      return 'test contents'
+   })
+
    it('should extract filenames correctly', () => {
       expect(extractFileNames(testString)).toEqual([
-         'testdir/test.yml',
-         'test2.yml',
-         'testdir/subdir/test3.yml',
-         'test4.yml',
-         'test5.yml'
+         '/tmp/testdir/test.yml',
+         '/tmp/test2.yml',
+         '/tmp/testdir/subdir/test3.yml',
+         '/tmp/test4.yml',
+         '/tmp/test5.yml'
       ])
    })
 
-   it('should replace filenames with basenames correctly', () => {
-      expect(replaceFileNamesWithBaseNames(testString)).toEqual(
-         `kubectl annotate -f test.yml,test2.yml,test3.yml -f test4.yml --filename test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/manifests/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
+   it('should replace filenames with shallow names for relative locations in tmp correctly', () => {
+      expect(
+         replaceFileNamesWithShallowNamesRelativeToTemp(testString)
+      ).toEqual(
+         `kubectl annotate -f testdir-test.yml,test2.yml,testdir-subdir-test3.yml -f test4.yml --filename test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
       )
    })
 

diff --git a/src/types/privatekubectl.ts b/src/types/privatekubectl.ts
@@ -5,6 +5,7 @@ import * as core from '@actions/core'
 import * as os from 'os'
 import * as fs from 'fs'
 import * as path from 'path'
+import {getTempDirectory} from '../utilities/fileUtils'
 
 export class PrivateKubectl extends Kubectl {
    protected async execute(args: string[], silent: boolean = false) {
@@ -18,8 +19,7 @@ export class PrivateKubectl extends Kubectl {
       }
 
       if (this.containsFilenames(kubectlCmd)) {
-         // For private clusters, files will referenced solely by their basename
-         kubectlCmd = replaceFileNamesWithBaseNames(kubectlCmd)
+         kubectlCmd = replaceFileNamesWithShallowNamesRelativeToTemp(kubectlCmd)
          addFileFlag = true
       }
 
@@ -43,22 +43,9 @@ export class PrivateKubectl extends Kubectl {
       ]
 
       if (addFileFlag) {
-         const filenames = extractFileNames(kubectlCmd)
-
-         const tempDirectory =
-            process.env['runner.tempDirectory'] || os.tmpdir() + '/manifests'
-         eo.cwd = tempDirectory
+         const tempDirectory = getTempDirectory()
+         eo.cwd = path.join(tempDirectory, 'manifests')
          privateClusterArgs.push(...['--file', '.'])
-
-         for (const filename of filenames) {
-            try {
-               this.moveFileToTempManifestDir(filename)
-            } catch (e) {
-               core.debug(
-                  `Error moving file ${filename} to temp directory: ${e}`
-               )
-            }
-         }
       }
 
       core.debug(
@@ -98,63 +85,58 @@ export class PrivateKubectl extends Kubectl {
    private containsFilenames(str: string) {
       return str.includes('-f ') || str.includes('filename ')
    }
+}
 
-   private createTempManifestsDirectory() {
-      const manifestsDir = '/tmp/manifests'
-      if (!fs.existsSync('/tmp/manifests')) {
-         fs.mkdirSync('/tmp/manifests', {recursive: true})
-      }
+function createTempManifestsDirectory(): string {
+   const manifestsDirPath = path.join(getTempDirectory(), 'manifests')
+   if (!fs.existsSync(manifestsDirPath)) {
+      fs.mkdirSync(manifestsDirPath, {recursive: true})
    }
 
-   private moveFileToTempManifestDir(file: string) {
-      this.createTempManifestsDirectory()
-      if (!fs.existsSync('/tmp/' + file)) {
-         core.debug(
-            '/tmp/' +
-               file +
-               ' does not exist, and therefore cannot be moved to the manifest directory'
-         )
-      }
-
-      fs.copyFile('/tmp/' + file, '/tmp/manifests/' + file, function (err) {
-         if (err) {
-            core.debug(
-               'Could not rename ' +
-                  '/tmp/' +
-                  file +
-                  ' to  ' +
-                  '/tmp/manifests/' +
-                  file +
-                  ' ERROR: ' +
-                  err
-            )
-            return
-         }
-         core.debug(
-            "Successfully moved file '" +
-               file +
-               "' from /tmp to /tmp/manifest directory"
-         )
-      })
-   }
+   return manifestsDirPath
 }
 
-export function replaceFileNamesWithBaseNames(kubectlCmd: string) {
+export function replaceFileNamesWithShallowNamesRelativeToTemp(
+   kubectlCmd: string
+) {
    let filenames = extractFileNames(kubectlCmd)
-   let basenames = filenames.map((filename) => path.basename(filename))
+   core.debug(`filenames originally provided in kubectl command: ${filenames}`)
+   let relativeShallowNames = filenames.map((filename) => {
+      const relativeName = path.relative(getTempDirectory(), filename)
+
+      const relativePathElements = relativeName.split(path.sep)
+
+      const shallowName = relativePathElements.join('-')
+
+      // make manifests dir in temp if it doesn't already exist
+      const manifestsTempDir = createTempManifestsDirectory()
+
+      const shallowPath = path.join(manifestsTempDir, shallowName)
+      core.debug(
+         `moving contents from ${filename} to shallow location at ${shallowPath}`
+      )
+
+      core.debug(`reading contents from ${filename}`)
+      const contents = fs.readFileSync(filename).toString()
+
+      core.debug(`writing contents to new path ${shallowPath}`)
+      fs.writeFileSync(shallowPath, contents)
+
+      return shallowName
+   })
 
    let result = kubectlCmd
-   if (filenames.length != basenames.length) {
+   if (filenames.length != relativeShallowNames.length) {
       throw Error(
-         'replacing filenames with basenames, ' +
+         'replacing filenames with relative path from temp dir, ' +
             filenames.length +
             ' filenames != ' +
-            basenames.length +
+            relativeShallowNames.length +
             'basenames'
       )
    }
    for (let index = 0; index < filenames.length; index++) {
-      result = result.replace(filenames[index], basenames[index])
+      result = result.replace(filenames[index], relativeShallowNames[index])
    }
    return result
 }

diff --git a/src/utilities/fileUtils.test.ts b/src/utilities/fileUtils.test.ts
@@ -1,20 +1,14 @@
-import {
-   getFilesFromDirectoriesAndURLs,
-   getTempDirectory,
-   urlFileKind,
-   writeYamlFromURLToFile
-} from './fileUtils'
+import * as fileUtils from './fileUtils'
 
 import * as yaml from 'js-yaml'
 import * as fs from 'fs'
 import * as path from 'path'
-import {succeeded} from '../types/errorable'
 
 const sampleYamlUrl =
    'https://raw.githubusercontent.com/kubernetes/website/main/content/en/examples/controllers/nginx-deployment.yaml'
 describe('File utils', () => {
    test('correctly parses a yaml file from a URL', async () => {
-      const tempFile = await writeYamlFromURLToFile(sampleYamlUrl, 0)
+      const tempFile = await fileUtils.writeYamlFromURLToFile(sampleYamlUrl, 0)
       const fileContents = fs.readFileSync(tempFile).toString()
       const inputObjects = yaml.safeLoadAll(fileContents)
       expect(inputObjects).toHaveLength(1)
@@ -30,34 +24,34 @@ describe('File utils', () => {
 
       const testPath = path.join('test', 'unit', 'manifests')
       await expect(
-         getFilesFromDirectoriesAndURLs([testPath, badUrl])
+         fileUtils.getFilesFromDirectoriesAndURLs([testPath, badUrl])
       ).rejects.toThrow()
    })
 
-   it('detects files in nested directories and ignores non-manifest files and empty dirs', async () => {
+   it('detects files in nested directories with the same name and ignores non-manifest files and empty dirs', async () => {
       const testPath = path.join('test', 'unit', 'manifests')
-      const testSearch: string[] = await getFilesFromDirectoriesAndURLs([
-         testPath,
-         sampleYamlUrl
-      ])
+      const testSearch: string[] =
+         await fileUtils.getFilesFromDirectoriesAndURLs([
+            testPath,
+            sampleYamlUrl
+         ])
 
       const expectedManifests = [
-         'test/unit/manifests/manifest_test_dir/another_layer/deep-ingress.yaml',
-         'test/unit/manifests/manifest_test_dir/another_layer/deep-service.yaml',
+         'test/unit/manifests/manifest_test_dir/another_layer/test-ingress.yaml',
+         'test/unit/manifests/manifest_test_dir/another_layer/nested-test-service.yaml',
          'test/unit/manifests/manifest_test_dir/nested-test-service.yaml',
          'test/unit/manifests/test-ingress.yml',
          'test/unit/manifests/test-ingress-new.yml',
          'test/unit/manifests/test-service.yml'
       ]
 
-      // is there a more efficient way to test equality w random order?
       expect(testSearch).toHaveLength(8)
       expectedManifests.forEach((fileName) => {
          if (fileName.startsWith('test/unit')) {
             expect(testSearch).toContain(fileName)
          } else {
-            expect(fileName.includes(urlFileKind)).toBe(true)
-            expect(fileName.startsWith(getTempDirectory()))
+            expect(fileName.includes(fileUtils.urlFileKind)).toBe(true)
+            expect(fileName.startsWith(fileUtils.getTempDirectory()))
          }
       })
    })
@@ -72,7 +66,7 @@ describe('File utils', () => {
       )
 
       expect(
-         getFilesFromDirectoriesAndURLs([badPath, goodPath])
+         fileUtils.getFilesFromDirectoriesAndURLs([badPath, goodPath])
       ).rejects.toThrowError()
    })
 
@@ -92,7 +86,7 @@ describe('File utils', () => {
       )
 
       expect(
-         await getFilesFromDirectoriesAndURLs([
+         await fileUtils.getFilesFromDirectoriesAndURLs([
             outerPath,
             fileAtOuter,
             innerPath
@@ -102,6 +96,24 @@ describe('File utils', () => {
 
    it('throws an error for an invalid URL', async () => {
       const badUrl = 'https://www.github.com'
-      await expect(writeYamlFromURLToFile(badUrl, 0)).rejects.toBeTruthy()
+      await expect(
+         fileUtils.writeYamlFromURLToFile(badUrl, 0)
+      ).rejects.toBeTruthy()
+   })
+})
+
+describe('moving files to temp', () => {
+   it('correctly moves the contents of a file to the temporary directory', () => {
+      jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {})
+      jest.spyOn(fs, 'readFileSync').mockImplementation((filename) => {
+         return 'test contents'
+      })
+      const originalFilePath = path.join('path', 'in', 'repo')
+
+      const output = fileUtils.moveFileToTmpDir(originalFilePath)
+
+      expect(output).toEqual(
+         path.join(fileUtils.getTempDirectory(), '/path/in/repo')
+      )
    })
 })