Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
INT-2469 Add cache when using --step option #606
INT-2469 Add cache when using --step option #606
Changes from 5 commits
506870f
ae597dd
503af54
b130c2e
e45446d
493edb6
1fc882f
f5c94e3
f670b57
fc3746e
b379bd7
7b24e4c
68e89b5
50301aa
7902f78
3c15357
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
👍
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.
Think this is a typo here.
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.
Was
.j1-integration-cache
considered? I think it would be nice to see it right next to.j1-integration
in file listings.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.
Agreed. @VDubber and I talked about this too.
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.
Great minds with the same idea. I'll make it happen.
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.
You'll probably want to go back to the
integration-template
project and update the.gitignore
to include.j1-cache
.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.
@VDubber We'll probably want to update this to reflect the new
/graph
nesting.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.
I'm thinking through the file system here... Seems that I can also do the following as many times as I want?
--use-dependencies-cache
option to gather data in .j1-integration--step
and--use-dependencies-cache
option without specifying a filepath.This will cause the .j1-integration data to populate the .j1-cache.
Is it true that I can continue to repeat step 2 over and over and it will only ever actually re-ingest the
--step fetch-users
step?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.
I have the same question as Nick here. If this is true, I suppose running step 2 is essentially "refreshing the cache". I was going to suggest we add an option to clear and refresh the cache but sounds like maybe this would handle the refreshing part anyway. I'm just not sure how intuitive that is vs a separate flag needed to refresh the cache in here and otherwise it will default to using the cache that already lives in the directory. Thoughts?
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.
@ndowmon Yes. if you repeat step 2 it will copy over the .j1-integration data over and over and only execute the fetch-users step. If the .j1-integration data goes sideways, you'll end up with a cache that is busted. Hence the suggestion to specify the filepath - then it'll stop trying to create the cache.
@ceelias It isn't clear. I thought about a command as well but that seemed too much. I imagine caches being self-maintaining. Not sure if that's the correct approach. Right now the cache creation destroys the previous cache. We could do more of an update, maybe?.... That might be tricky to get right.
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.
@ndowmon @ceelias
Crazy idea: what if we turn on caching by default whenever the
--step
flag is used? We could introduce a simple--no-cache
flag and-cache-path
flag. I think this make the cache helpful and out of the way until you need to deal with it.The --step flag is already used to decrease developer wait time... Why not make it even better with the cache?
On the initial run, when there is no .j1-integration data, it doesn't utilize the cache...
--
I might be in cacheland for too long now and my mind is skewed.
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.
🤔 hm, that is an interesting idea... although I do think you may be stuck in cacheland 😉
My concern with defaulting to
--no-cache
is that differences in upstream dependencies may (or probably should) cause the chosen--step
to behave differently. I still think it's best to refresh the dependent steps before running the new step.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.
I may have missed it but what happens in the case that the integration wasn't run yet and there is no data to copy over to the cache? Might be worth a test here.
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.
If a step is tagged as using the cache but fails to load any entities/relationships, it marks that step as a failure. I'll see if I can get that in here.
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.
I added a test and it caused the other tests to fail. I may come back to this at another time.
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.
More so for curiousity but we have the ability to create multiple dependency graphs. Have you tried to run that use case/does your code account for that? Maybe something we can address in a later PR if not.
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.
I have not. I'd love to see that in action!
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.
Opened #619 to track
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.
This seems like it is an async function? See: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/fs-extra/index.d.ts#L45
Can we install
@types/fs-extra
as a dev dep?