-
Notifications
You must be signed in to change notification settings - Fork 18
fix(cli): fix cache .gitignore written to fs root and undefined workflow name #1444
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
base: main
Are you sure you want to change the base?
Changes from all commits
4d8dc20
03f48ec
2968277
1e47fad
9817847
d0518f9
d4f0e3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -418,6 +418,55 @@ test.serial('.cli-cache has a gitignore', async (t) => { | |
| t.is(gitignore, '*'); | ||
| }); | ||
|
|
||
| // Regression test for https://github.com/OpenFn/kit/issues/669 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this test making such a distinction between workflow.js and expression.js? I don't think it materially affects the cache behaviour? |
||
| test.serial('cache steps when running a .js expression', async (t) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test just ensures that the cache is written, not read. Which is fine - I'd just like the test name to be accurate. |
||
| mockFs({ | ||
| '/job.js': `${fn}fn((state) => ({ ...state, x: 1 }));`, | ||
| }); | ||
|
|
||
| const options = { | ||
| ...defaultOptions, | ||
| expressionPath: '/job.js', | ||
| baseDir: '/', | ||
| cacheSteps: true, | ||
| }; | ||
|
|
||
| const result = await handler(options, logger); | ||
| t.is(result.x, 1); | ||
|
|
||
| // workflow name is derived from the filename: 'job' | ||
| // step id is a generated uuid, so list the directory | ||
| const files = await fs.readdir('/.cli-cache/job'); | ||
| t.is(files.length, 1); | ||
| t.true(files[0].endsWith('.json')); | ||
|
|
||
| const cached = JSON.parse( | ||
| await fs.readFile(`/.cli-cache/job/${files[0]}`, 'utf8') | ||
| ); | ||
| t.is(cached.x, 1); | ||
| }); | ||
|
|
||
| test.serial( | ||
| '.cli-cache has a gitignore when caching a .js expression', | ||
| async (t) => { | ||
| mockFs({ | ||
| '/job.js': `${fn}fn((state) => ({ ...state, x: 1 }));`, | ||
| }); | ||
|
|
||
| const options = { | ||
| ...defaultOptions, | ||
| expressionPath: '/job.js', | ||
| baseDir: '/', | ||
| cacheSteps: true, | ||
| }; | ||
|
|
||
| await handler(options, logger); | ||
|
|
||
| const gitignore = await fs.readFile('/.cli-cache/.gitignore', 'utf8'); | ||
| t.is(gitignore, '*'); | ||
| } | ||
| ); | ||
|
|
||
| test.serial('run a workflow with initial state from stdin', async (t) => { | ||
| const workflow = { | ||
| workflow: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old logic here, the reason we walk up through the folder structure, is to see if there's a parenting .gitignore file which we can edit. This would live at the root of the github repo, which we may or may not find.
This PR changes the approach to force a .gitignore file as a child of the cache folder.
I'm not against this. It might be a bit messy but I suppose it's not too big a deal. And I can see the original issue of writing a
.gitignorefile to root (although I can't reproduce this locally but that might be something in my local setup)So Ok, let's do it!