Remove followLogs function

Remove the `followLogs` function and update `fetchLogs` to support the `stream` parameter. There's no need to maintain 2 separate functions for this. Update tests and usage accordingly.
tektoncd · Jan 8, 2025 · 1c53126 · 1c53126
1 parent bea10b7
commit 1c53126
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 84 deletions.
diff --git a/packages/components/src/components/PipelineRun/PipelineRun.jsx b/packages/components/src/components/PipelineRun/PipelineRun.jsx
@@ -120,7 +120,7 @@ export default /* istanbul ignore next */ function PipelineRun({
               taskRun
             })
           }
-          fetchLogs={() => fetchLogs(stepName, stepStatus, taskRun)}
+          fetchLogs={() => fetchLogs({ stepName, stepStatus, taskRun })}
           forcePolling={forceLogPolling}
           key={`${selectedTaskId}:${selectedStepId}:${selectedRetry}`}
           logLevels={logLevels}

diff --git a/src/containers/TaskRun/TaskRun.jsx b/src/containers/TaskRun/TaskRun.jsx
@@ -182,10 +182,12 @@ export function TaskRunContainer({
           : null)}
       >
         <Log
-          fetchLogs={() => logsRetriever(stepName, stepStatus, run)}
-          isLogsMaximized={isLogsMaximized}
           enableLogAutoScroll
           enableLogScrollButtons
+          fetchLogs={() =>
+            logsRetriever({ stepName, stepStatus, taskRun: run })
+          }
+          isLogsMaximized={isLogsMaximized}
           key={`${stepName}:${currentRetry}`}
           logLevels={logLevels}
           showLevels={showLogLevels}

diff --git a/src/utils/index.js b/src/utils/index.js
@@ -55,7 +55,7 @@ export function sortRunsByCreationTime(runs) {
   });
 }
 
-export async function followLogs(stepName, stepStatus, taskRun) {
+export async function fetchLogs({ _stepName, stream, stepStatus, taskRun }) {
   const { namespace } = taskRun.metadata;
   const { podName } = taskRun.status || {};
   let logs;
@@ -65,22 +65,7 @@ export async function followLogs(stepName, stepStatus, taskRun) {
       container,
       name: podName,
       namespace,
-      stream: true
-    });
-  }
-  return logs;
-}
-
-export async function fetchLogs(stepName, stepStatus, taskRun) {
-  const { namespace } = taskRun.metadata;
-  const { podName } = taskRun.status || {};
-  let logs;
-  if (podName && stepStatus) {
-    const { container } = stepStatus;
-    logs = getPodLog({
-      container,
-      name: podName,
-      namespace
+      stream
     });
   }
   return logs;
@@ -91,7 +76,7 @@ export function fetchLogsFallback(externalLogsURL) {
     return undefined;
   }
 
-  return (stepName, stepStatus, taskRun) => {
+  return ({ _stepName, stepStatus, taskRun }) => {
     const { namespace } = taskRun.metadata;
     const { podName, startTime, completionTime } = taskRun.status || {};
     const { container } = stepStatus;
@@ -116,18 +101,23 @@ export function getLogsRetriever({
   isLogStreamingEnabled,
   onFallback
 }) {
-  const logs = isLogStreamingEnabled ? followLogs : fetchLogs;
   const fallback = fetchLogsFallback(externalLogsURL);
 
   if (fallback) {
-    return (stepName, stepStatus, taskRun) =>
-      logs(stepName, stepStatus, taskRun).catch(() => {
+    return ({ stepName, stepStatus, taskRun }) =>
+      fetchLogs({
+        stepName,
+        stream: isLogStreamingEnabled,
+        stepStatus,
+        taskRun
+      }).catch(() => {
         onFallback(true);
-        return fallback(stepName, stepStatus, taskRun);
+        // TODO: logs - support streaming from external logs?
+        return fallback({ stepName, stepStatus, taskRun });
       });
   }
 
-  return logs;
+  return fetchLogs;
 }
 
 // K8s label documentation comes from here:

diff --git a/src/utils/index.test.js b/src/utils/index.test.js
@@ -16,7 +16,6 @@ import * as comms from '../api/comms';
 import {
   fetchLogs,
   fetchLogsFallback,
-  followLogs,
   getLocale,
   getLogsRetriever,
   getTheme,
@@ -101,7 +100,7 @@ describe('fetchLogs', () => {
     const logs = 'fake logs';
     vi.spyOn(API, 'getPodLog').mockImplementation(() => logs);
 
-    const returnedLogs = fetchLogs(stepName, stepStatus, taskRun);
+    const returnedLogs = fetchLogs({ stepName, stepStatus, taskRun });
     expect(API.getPodLog).toHaveBeenCalledWith(
       expect.objectContaining({
         container: stepStatus.container,
@@ -120,7 +119,60 @@ describe('fetchLogs', () => {
     const taskRun = { metadata: { namespace: 'default' } };
     vi.spyOn(API, 'getPodLog');
 
-    fetchLogs(stepName, stepStatus, taskRun);
+    fetchLogs({ stepName, stepStatus, taskRun });
+    expect(API.getPodLog).not.toHaveBeenCalled();
+  });
+
+  it('should return the streamed pod logs', () => {
+    const namespace = 'default';
+    const podName = 'pipeline-run-123456';
+    const stepName = 'kubectl-apply';
+    const stepStatus = { container: 'step-kubectl-apply' };
+    const taskRun = {
+      metadata: { namespace },
+      status: { podName }
+    };
+
+    const logs = {
+      getReader() {
+        return {
+          read() {
+            return Promise.resolve({
+              done: true,
+              value: new TextEncoder().encode('fake logs')
+            });
+          }
+        };
+      }
+    };
+    vi.spyOn(API, 'getPodLog').mockImplementation(() => logs);
+
+    const returnedLogs = fetchLogs({
+      stepName,
+      stream: true,
+      stepStatus,
+      taskRun
+    });
+    expect(API.getPodLog).toHaveBeenCalledWith(
+      expect.objectContaining({
+        container: stepStatus.container,
+        name: podName,
+        namespace,
+        stream: true
+      })
+    );
+    returnedLogs.then(data => {
+      expect(data).toBe(logs);
+    });
+  });
+
+  it('should not call the API when the pod is not specified', () => {
+    const stepName = 'kubectl-apply';
+    const stepStatus = { container: 'step-kubectl-apply' };
+    const taskRun = { metadata: { namespace: 'default' } };
+    vi.spyOn(API, 'getPodLog');
+
+    fetchLogs({ stepName, stream: true, stepStatus, taskRun });
     expect(API.getPodLog).not.toHaveBeenCalled();
   });
 });
@@ -146,7 +198,7 @@ describe('fetchLogsFallback', () => {
     vi.spyOn(comms, 'get').mockImplementation(() => {});
 
     const fallback = fetchLogsFallback(externalLogsURL);
-    fallback(stepName, stepStatus, taskRun);
+    fallback({ stepName, stepStatus, taskRun });
     expect(comms.get).toHaveBeenCalledWith(
       `http://localhost:3000${externalLogsURL}/${namespace}/${podName}/${container}?startTime=${startTime.replaceAll(
         ':',
@@ -167,64 +219,14 @@ describe('fetchLogsFallback', () => {
     vi.spyOn(comms, 'get').mockImplementation(() => {});
 
     const fallback = fetchLogsFallback(externalLogsURL);
-    fallback(stepName, stepStatus, taskRun);
+    fallback({ stepName, stepStatus, taskRun });
     expect(comms.get).toHaveBeenCalledWith(
       `http://localhost:3000${externalLogsURL}/${namespace}/${podName}/${container}`,
       { Accept: 'text/plain' }
     );
   });
 });
 
-describe('followLogs', () => {
-  it('should return the pod logs', () => {
-    const namespace = 'default';
-    const podName = 'pipeline-run-123456';
-    const stepName = 'kubectl-apply';
-    const stepStatus = { container: 'step-kubectl-apply' };
-    const taskRun = {
-      metadata: { namespace },
-      status: { podName }
-    };
-
-    const logs = {
-      getReader() {
-        return {
-          read() {
-            return Promise.resolve({
-              done: true,
-              value: new TextEncoder().encode('fake logs')
-            });
-          }
-        };
-      }
-    };
-    vi.spyOn(API, 'getPodLog').mockImplementation(() => logs);
-
-    const returnedLogs = followLogs(stepName, stepStatus, taskRun);
-    expect(API.getPodLog).toHaveBeenCalledWith(
-      expect.objectContaining({
-        container: stepStatus.container,
-        name: podName,
-        namespace,
-        stream: true
-      })
-    );
-    returnedLogs.then(data => {
-      expect(data).toBe(logs);
-    });
-  });
-
-  it('should not call the API when the pod is not specified', () => {
-    const stepName = 'kubectl-apply';
-    const stepStatus = { container: 'step-kubectl-apply' };
-    const taskRun = { metadata: { namespace: 'default' } };
-    vi.spyOn(API, 'getPodLog');
-
-    followLogs(stepName, stepStatus, taskRun);
-    expect(API.getPodLog).not.toHaveBeenCalled();
-  });
-});
-
 describe('getLogsRetriever', () => {
   const namespace = 'fake_namespace';
   const podName = 'fake_podName';
@@ -236,7 +238,7 @@ describe('getLogsRetriever', () => {
     vi.spyOn(API, 'getPodLog').mockImplementation(() => {});
     const logsRetriever = getLogsRetriever({});
     expect(logsRetriever).toBeDefined();
-    logsRetriever(stepName, stepStatus, taskRun);
+    logsRetriever({ stepName, stepStatus, taskRun });
     expect(API.getPodLog).toHaveBeenCalledWith({
       container: stepName,
       name: podName,
@@ -250,7 +252,7 @@ describe('getLogsRetriever', () => {
     const onFallback = vi.fn();
     const logsRetriever = getLogsRetriever({ externalLogsURL, onFallback });
     expect(logsRetriever).toBeDefined();
-    await logsRetriever(stepName, stepStatus, taskRun);
+    await logsRetriever({ stepName, stepStatus, taskRun });
     expect(API.getPodLog).toHaveBeenCalledWith({
       container: stepName,
       name: podName,
@@ -269,7 +271,7 @@ describe('getLogsRetriever', () => {
     const onFallback = vi.fn();
     const logsRetriever = getLogsRetriever({ externalLogsURL, onFallback });
     expect(logsRetriever).toBeDefined();
-    await logsRetriever(stepName, stepStatus, taskRun);
+    await logsRetriever({ stepName, stepStatus, taskRun });
     expect(API.getPodLog).toHaveBeenCalledWith({
       container: stepName,
       name: podName,