Skip to content
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.

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

engine: return slightly better credential errors #693

Merged
merged 6 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions integration-tests/worker/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# @openfn/integration-tests-worker

## 1.0.44

### Patch Changes

- Updated dependencies
- @openfn/[email protected]
- @openfn/[email protected]
- @openfn/[email protected]

## 1.0.43

### Patch Changes
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/worker/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@openfn/integration-tests-worker",
"private": true,
"version": "1.0.43",
"version": "1.0.44",
"description": "Lightning WOrker integration tests",
"author": "Open Function Group <[email protected]>",
"license": "ISC",
Expand Down
24 changes: 24 additions & 0 deletions integration-tests/worker/test/exit-reasons.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,30 @@ test('exception: autoinstall error', async (t) => {
);
});

test('exception: bad credential (not found)', async (t) => {
const attempt = {
id: crypto.randomUUID(),
jobs: [
{
adaptor: '@openfn/language-common@latest',
body: 'fn((s) => s)',
credential: 'been-to-the-mountain', // the mock will return not_found
},
],
};

const result = await run(attempt);

const { reason, error_type, error_message } = result;

t.is(reason, 'exception');
t.is(error_type, 'CredentialLoadError');
t.is(
error_message,
'Failed to load credential been-to-the-mountain: not_found'
);
});

test('kill: oom (small, kill worker)', async (t) => {
const attempt = {
id: crypto.randomUUID(),
Expand Down
5 changes: 4 additions & 1 deletion integration-tests/worker/test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,10 @@ test.serial('run a job with bad credentials', (t) => {
lightning.once('run:complete', ({ payload }) => {
t.is(payload.reason, 'exception');
t.is(payload.error_type, 'CredentialLoadError');
t.regex(payload.error_message, /Failed to load credential zzz for step/);
t.regex(
payload.error_message,
/Failed to load credential zzz: not_found/
);
done();
});

Expand Down
6 changes: 6 additions & 0 deletions packages/engine-multi/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# engine-multi

## 1.1.8

### Patch Changes

- Better error reporting for bad credentials

## 1.1.7

### Patch Changes
Expand Down
2 changes: 1 addition & 1 deletion packages/engine-multi/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@openfn/engine-multi",
"version": "1.1.7",
"version": "1.1.8",
"description": "Multi-process runtime engine",
"main": "dist/index.js",
"type": "module",
Expand Down
12 changes: 11 additions & 1 deletion packages/engine-multi/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,18 @@ export class CredentialLoadError extends EngineError {
original: any; // this is the original error
constructor(errors: CredentialErrorObj[]) {
super();
const ids: Record<string, true> = {};
this.message = errors
.map((e) => `Failed to load credential ${e.id} for step ${e.step}`)
.filter((e) => {
if (!ids[e.id]) {
ids[e.id] = true;
return true;
}
})
.map(
(e) =>
`Failed to load credential ${e.id}${e.error ? `: ${e.error}` : ''}`
)
.join('\n');
}
}
40 changes: 36 additions & 4 deletions packages/engine-multi/test/api/preload-credentials.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ test('throw if one credential fails to load', async (t) => {
await preloadCredentials(plan, loader);
} catch (e: any) {
t.is(e.name, 'CredentialLoadError');
t.is(e.message, `Failed to load credential a for step z`);
t.is(e.message, `Failed to load credential a: err`);
}
});

Expand All @@ -118,7 +118,7 @@ test('throw if several credentials fail to load', async (t) => {
{
id: 'k',
expression: '.',
configuration: 'a',
configuration: 'b',
},
],
},
Expand All @@ -131,8 +131,40 @@ test('throw if several credentials fail to load', async (t) => {
t.is(e.name, 'CredentialLoadError');
t.is(
e.message,
`Failed to load credential a for step j
Failed to load credential a for step k`
`Failed to load credential a: err
Failed to load credential b: err`
);
}
});

test('only report each credential once', async (t) => {
const loader = async () => {
throw new Error('err');
};

const plan = {
id: t.title,
workflow: {
steps: [
{
id: 'j',
expression: '.',
configuration: 'a',
},
{
id: 'k',
expression: '.',
configuration: 'a',
},
],
},
options: {},
} as ExecutionPlan;

try {
await preloadCredentials(plan, loader);
} catch (e: any) {
t.is(e.name, 'CredentialLoadError');
t.is(e.message, `Failed to load credential a: err`);
}
});
2 changes: 1 addition & 1 deletion packages/engine-multi/test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ test.serial('send a workflow error if credentials fail to load', (t) => {

t.is(e.type, 'CredentialLoadError');
t.is(e.severity, 'exception');
t.is(e.message, 'Failed to load credential secret for step a');
t.is(e.message, 'Failed to load credential secret');
done();
});
});
Expand Down
7 changes: 7 additions & 0 deletions packages/lightning-mock/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# @openfn/lightning-mock

## 2.0.8

### Patch Changes

- Updated dependencies
- @openfn/[email protected]

## 2.0.7

### Patch Changes
Expand Down
2 changes: 1 addition & 1 deletion packages/lightning-mock/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@openfn/lightning-mock",
"version": "2.0.7",
"version": "2.0.8",
"private": true,
"description": "A mock Lightning server",
"main": "dist/index.js",
Expand Down
8 changes: 8 additions & 0 deletions packages/ws-worker/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# ws-worker

## 1.1.9

### Patch Changes

- Better error reporting for bad credentials
- Updated dependencies
- @openfn/[email protected]

## 1.1.8

### Patch Changes
Expand Down
2 changes: 1 addition & 1 deletion packages/ws-worker/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@openfn/ws-worker",
"version": "1.1.8",
"version": "1.1.9",
"description": "A Websocket Worker to connect Lightning to a Runtime Engine",
"main": "dist/index.js",
"type": "module",
Expand Down
2 changes: 1 addition & 1 deletion packages/ws-worker/test/reasons.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ test('exception: failed to load credential', async (t) => {

t.is(reason.reason, 'exception');
t.is(reason.error_type, 'CredentialLoadError');
t.is(reason.error_message, 'Failed to load credential zzz for step aa');
t.is(reason.error_message, 'Failed to load credential zzz: err');
});

test('kill: timeout', async (t) => {
Expand Down