Support file path in job body #742

midigofrank · 2024-08-09T20:32:42Z

Short Description

Support file paths in job body

Related issue

Fixes #285

Implementation Details

When the path is present in the body, we load the file's content into another key content. So essentially, it becomes:
{ path: './bodyPath.js', content: 'content fron bodyPath.js' }

The YAML.vistor function does all the magic here.
While traversing the nodes, we check if the current node is the job body. We know that it's the job body by moving up the path tree until we find a node with the key jobs. Here's how the path tree looks like:

[
  Document {
    ....
    contents: YAMLMap { items: [Array], range: [Array] },
    range: [ 0, 972, 972 ]
  },
  YAMLMap { items: [ [Pair], [Pair] ], range: [ 0, 972, 972 ] },
  Pair {
    key: Scalar {
      value: 'workflows',
      ...
    },
    value: YAMLMap { items: [Array], range: [Array] }
  },
  YAMLMap { items: [ [Pair] ], range: [ 74, 972, 972 ] },
  Pair {
    key: Scalar {
      value: 'DHIS2-to-Sheets',
      ...
    },
    value: YAMLMap { items: [Array], range: [Array] }
  },
  YAMLMap {
    items: [ [Pair], [Pair], [Pair], [Pair] ],
    range: [ 95, 972, 972 ]
  },
  Pair {
    key: Scalar {
      value: 'jobs',
      ...
    },
    value: YAMLMap { items: [Array], range: [Array] }
  },
  YAMLMap { items: [ [Pair], [Pair] ], range: [ 133, 526, 526 ] },
  Pair {
    key: Scalar {
      value: 'Upload-to-Google-Sheet',
      ...
    },
    value: YAMLMap { items: [Array], range: [Array] }
  },
  YAMLMap {
    items: [ [Pair], [Pair], [Pair] ],
    range: [ 363, 526, 526 ]
  }
]

When you pull, we merge the StateJob.id into the SpecJob.id of the current state and spec in place. We then keep track of all the jobs in a single place. The id here is guaranteed to be unique.
These "old spec jobs" are what we use to match the "new spec job".

QA Notes

List any considerations/cases/advice for testing/QA here.

Checklist before requesting a review

I have performed a self-review of my code
I have added unit tests
Changesets have been added (if there are production code changes)

taylordowns2000

looks great @midigofrank , can you:

update test/fixtures.ts to include relative paths? (and ensure the tests work properly)
edit https://github.com/OpenFn/docs/blob/main/docs/deploy/portability.md?plain=1#L31 to show the new format?

don't worry about shifting the current spec to "old versions"—i'll clean that up myself once your PR is open on docs

midigofrank · 2024-08-11T14:58:06Z

looks great @midigofrank , can you:

update test/fixtures.ts to include relative paths? (and ensure the tests work properly)

edit https://github.com/OpenFn/docs/blob/main/docs/deploy/portability.md?plain=1#L31 to show the new format?

don't worry about shifting the current spec to "old versions"—i'll clean that up myself once your PR is open on docs

@taylordowns2000 the test/fixtures.ts are only used in testing the stateTransform functionality. State transform happens waay after the relative paths have been used to load the body.

What I can test however is that if the body is in the form:

body: 
  content: "some content"

Then the tests would still pass

josephjclark

Hi @midigofrank, sorry I've been so late getting back to you on this. A few comments from me!

I'm getting more and more worried about complexity in deploy. If we have time I'd love to see some fresh unit tests on the new functions.

josephjclark · 2024-08-12T16:44:38Z

.changeset/swift-dingos-know.md

+'@openfn/deploy': minor
+---
+
+Support file paths for job body


Can we have a slightly more user friendly release note for the CLI? Just needs a bit more context. Deploy: allow job body to be loaded from a file path in workflow.yaml would probably do

josephjclark · 2024-08-12T16:45:30Z

packages/cli/src/pull/handler.ts

-      path.resolve(config.statePath),
-      JSON.stringify(state, null, 2)
-    );
+    // defer writing to disk until we have the spec


No need for this comment to stay in the source, it's a bit confusing without the diff tbh

josephjclark · 2024-08-12T16:49:44Z

packages/deploy/src/index.ts

@@ -91,7 +91,7 @@ function writeState(config: DeployConfig, nextState: {}): Promise<void> {
 export async function getSpec(path: string) {
  try {
    const body = await readFile(path, 'utf8');
-    return parseAndValidate(body);
+    return await parseAndValidate(body, path);


Don't return and await please!

An async function returns a promise, so you can just return the parseAndValidate promise straight out of the function

josephjclark · 2024-08-12T16:51:27Z

packages/deploy/src/index.ts

+  const jobs: SpecJob[] = [];
+
+  try {
+    const [state, spec] = await Promise.all([


You don't think this is cleaner?

const state = await getState(config.statePath) const spec = await getSpec(config.specPath)

Up to you!

josephjclark · 2024-08-12T17:13:39Z

packages/deploy/test/validator.test.ts

+import path from 'path';
+
+// Helper to create and clean up temporary test files
+const createTempFile = async (t, content: string, ext = 'txt') => {


hmm. There is a very clean file system mock in use in this repo which would make these tests a lot nicer IMO.

We use mock-fs

It can get a bit hairy when it blocks out node modules, but that can be worked around. See the mockFs helper in cli/test/util.ts (you can't import that into deploy but feel free to duplicate it )

There's some simple example usage in runtime/test/repo/ensure-repo.test.ts

Not neccessary if you're struggling for time, but strongly recommended

Niice, I'll change it

josephjclark · 2024-08-12T17:16:12Z

packages/deploy/src/validator.ts

-export function parseAndValidate(input: string): {
+export async function parseAndValidate(
+  input: string,
+  specPath: string


I think you should default specPath to '.' - that way you don't break unit tests which don't use it

josephjclark · 2024-08-12T17:53:16Z

packages/deploy/src/index.ts

+  return jobs;
+}
+
+export async function updatePulledSpecJobBodyPath(


Can I ask for a better function name here? And maybe on addSpecJobBodyPath ?

This is really complex stuff and it's taken me ages to get any kind of feeling for what's happening here 😅

Basically what these functions are doing is pulling out job bodies from the newly pulled spec and writing them to disk if the state (?) includes a path. Right?

So something like extractJobsToDisk might be a better name for addSpecJobBodyPath

Probably updatePulledSpecJobBodyPath should be moved to a new file packages/deploy/src/pull.ts, since it only affects pulls the context helps understandability. A name like syncRemoteSpec or something might be a good general name for what this is trying to do

midigofrank added 2 commits August 9, 2024 23:10

support file paths

69a37b3

add changeset file

2ca8bab

midigofrank self-assigned this Aug 9, 2024

add test

3440a4f

midigofrank requested review from josephjclark and stuartc August 9, 2024 21:29

taylordowns2000 requested changes Aug 10, 2024

View reviewed changes

refactor for easy readability

ba6251a

update fixtures to use body content

b51407d

midigofrank mentioned this pull request Aug 11, 2024

Showcase support for file paths in project-spec.yaml OpenFn/docs#533

Merged

midigofrank requested a review from taylordowns2000 August 11, 2024 15:26

josephjclark reviewed Aug 12, 2024

View reviewed changes

move relevant functions to pull.ts

222d05f

stuartc approved these changes Aug 14, 2024

View reviewed changes

stuartc merged commit b7fc4d0 into main Aug 14, 2024
6 checks passed

stuartc deleted the support-file-path branch August 14, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support file path in job body #742

Support file path in job body #742

midigofrank commented Aug 9, 2024 •

edited

Loading

taylordowns2000 left a comment

midigofrank commented Aug 11, 2024

josephjclark left a comment

josephjclark Aug 12, 2024

josephjclark Aug 12, 2024

josephjclark Aug 12, 2024

josephjclark Aug 12, 2024

josephjclark Aug 12, 2024

midigofrank Aug 13, 2024

josephjclark Aug 12, 2024

josephjclark Aug 12, 2024

Support file path in job body #742

Support file path in job body #742

Conversation

midigofrank commented Aug 9, 2024 • edited Loading

Short Description

Related issue

Implementation Details

QA Notes

Checklist before requesting a review

taylordowns2000 left a comment

Choose a reason for hiding this comment

midigofrank commented Aug 11, 2024

josephjclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midigofrank commented Aug 9, 2024 •

edited

Loading