Skip to content

chore: remove localhost value for AGENT_INTERNAL_URL #224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
josephemswiler wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: remove localhost value for AGENT_INTERNAL_URL #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90702e7
chore: remove localhost value for AGENT_INTERNAL_URL
josephemswiler Nov 18, 2024
5f10d65
chore: add value to tests for env variable AGENT_INTERNAL_URL
josephemswiler Nov 26, 2024
b25ace3
chore: add test for nullish AGENT_INTERNAL_URL value
josephemswiler Nov 26, 2024
98136eb
chore: update tests for AGENT_INTERNAL_URL
josephemswiler Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/configure/src/utils/authenticated.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import { Configuration, DefaultApi } from '@flatfile/api'
// TODO: We will need to make this conditional depending on if it's in the NodeVM or the Browser
import fetch, { RequestInit } from 'node-fetch'

const FLATFILE_API_URL =
process.env.AGENT_INTERNAL_URL || 'http://localhost:3000'
const FLATFILE_API_URL = process.env.AGENT_INTERNAL_URL

if (FLATFILE_API_URL == null) {
throw new Error('AGENT_INTERNAL_URL must be set in the environment')
}

export class AuthenticatedClient {
private _api?: DefaultApi
Expand Down
8 changes: 6 additions & 2 deletions packages/listener/src/events/authenticated.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ export class AuthenticatedClient {
public _apiUrl?: string

constructor(accessToken?: string, apiUrl?: string) {
const FLATFILE_API_URL =
CrossEnvConfig.get('AGENT_INTERNAL_URL') || 'http://localhost:3000'
const FLATFILE_API_URL = CrossEnvConfig.get('AGENT_INTERNAL_URL')

if (FLATFILE_API_URL == null) {
throw new Error('AGENT_INTERNAL_URL must be set in the environment')
}

const bearerToken = CrossEnvConfig.get('FLATFILE_BEARER_TOKEN')

this._accessToken = accessToken || bearerToken || '...'
Expand Down
20 changes: 20 additions & 0 deletions packages/listener/src/events/event.handler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { CrossEnvConfig } from '@flatfile/cross-env-config'
import { EventHandler } from './event.handler'

describe('EventHandler', () => {
let testFn: jest.Mock

if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}
Comment on lines +7 to +9
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Test setup needs revision to align with PR objectives.

The current test setup automatically sets a default value for AGENT_INTERNAL_URL, which contradicts the PR's objective of requiring explicit environment variable definition. According to the PR objectives, the application should throw an error when AGENT_INTERNAL_URL is not set.

Consider these improvements:

  1. Remove the default value assignment
  2. Add test cases to verify error handling when AGENT_INTERNAL_URL is not set
  3. Add test cases to verify successful operation with a valid URL

Here's a suggested approach:

- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
-   process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }

+ describe('AGENT_INTERNAL_URL validation', () => {
+   const originalEnv = process.env.AGENT_INTERNAL_URL;
+
+   afterEach(() => {
+     process.env.AGENT_INTERNAL_URL = originalEnv;
+   });
+
+   test('throws error when AGENT_INTERNAL_URL is not set', () => {
+     delete process.env.AGENT_INTERNAL_URL;
+     expect(() => new EventHandler()).toThrow('AGENT_INTERNAL_URL must be set in the environment');
+   });
+
+   test('creates handler successfully with valid AGENT_INTERNAL_URL', () => {
+     process.env.AGENT_INTERNAL_URL = 'http://test-agent:3000';
+     expect(() => new EventHandler()).not.toThrow();
+   });
+ });

Committable suggestion skipped: line range outside the PR's diff.


beforeEach(() => {
testFn = jest.fn()
})
Expand Down Expand Up @@ -86,4 +91,19 @@ describe('EventHandler', () => {
expect(testFn).toHaveBeenCalledTimes(1)
})
})

describe('AGENT_INTERNAL_URL', () => {
test('throws error when not set', () => {
process.env.AGENT_INTERNAL_URL = undefined

try {
new EventHandler().on('foo', testFn)
} catch (e) {
expect(e).toBeInstanceOf(Error)
expect((e as Error).message).toBe(
'AGENT_INTERNAL_URL must be set in the environment'
)
}
})
})
})
22 changes: 22 additions & 0 deletions packages/listener/src/flatfile.listener.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { CrossEnvConfig } from '@flatfile/cross-env-config'
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Test setup contradicts PR objectives

The current implementation sets a default value for AGENT_INTERNAL_URL when null, which contradicts the PR objectives stating that the application should throw an error when this variable is not set.

Consider replacing this with tests that verify both success and error cases:

-if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
-  process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
-}

+describe('AGENT_INTERNAL_URL validation', () => {
+  const originalEnv = process.env.AGENT_INTERNAL_URL;
+
+  afterEach(() => {
+    process.env.AGENT_INTERNAL_URL = originalEnv;
+  });
+
+  test('throws error when AGENT_INTERNAL_URL is not set', () => {
+    delete process.env.AGENT_INTERNAL_URL;
+    expect(() => {
+      FlatfileListener.create((c) => {
+        c.on('foo', () => {});
+      });
+    }).toThrow('AGENT_INTERNAL_URL must be set in the environment');
+  });
+
+  test('creates listener successfully when AGENT_INTERNAL_URL is set', () => {
+    process.env.AGENT_INTERNAL_URL = 'test_url';
+    expect(() => {
+      FlatfileListener.create((c) => {
+        c.on('foo', () => {});
+      });
+    }).not.toThrow();
+  });
+});

Also applies to: 7-9

import { FlatfileListener } from './flatfile.listener'

describe('Client', () => {
let testFn: jest.Mock

if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
}

beforeEach(() => {
testFn = jest.fn()
})
Expand Down Expand Up @@ -150,4 +155,21 @@ describe('Client', () => {
expect(testFn).toHaveBeenCalledTimes(3)
})
})

describe('AGENT_INTERNAL_URL', () => {
test('throws error when not set', () => {
process.env.AGENT_INTERNAL_URL = undefined

try {
FlatfileListener.create((c) => {
c.on('foo', () => {})
})
} catch (e) {
expect(e).toBeInstanceOf(Error)
expect((e as Error).message).toBe(
'AGENT_INTERNAL_URL must be set in the environment'
)
}
})
})
})