Skip to content

fix(cli): Use temporary wasm artifact_cache directory during validate command#4497

Closed
ktff wants to merge 5 commits intomasterfrom
ktff/tmp_file_wasm
Closed

fix(cli): Use temporary wasm artifact_cache directory during validate command#4497
ktff wants to merge 5 commits intomasterfrom
ktff/tmp_file_wasm

Conversation

@ktff
Copy link
Copy Markdown
Contributor

@ktff ktff commented Oct 11, 2020

Ref #3930

To avoid creating directories and caches in runtime directory we:

  • create them in a tmp directory
  • manually check original path

Open questions

  • Is there a better way to modify wasm transform configuration than passing BuildMode around?

ktff added 5 commits October 10, 2020 18:25
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
Signed-off-by: ktf <krunotf@gmail.com>
@ktff ktff added domain: cli Anything related to Vector's CLI transform: wasm labels Oct 11, 2020
@ktff ktff requested a review from Hoverbear October 11, 2020 12:55
@ktff ktff requested a review from lukesteensen as a code owner October 11, 2020 12:55
@ktff ktff self-assigned this Oct 11, 2020
@ktff
Copy link
Copy Markdown
Contributor Author

ktff commented Oct 16, 2020

cc @Hoverbear

Copy link
Copy Markdown
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

So, I'm worried this work will be for nothing as we plan to migrate to wasmtime in the future which does not have the concept of an artifact cache (it's a JIT with a store like https://docs.rs/wasmtime/0.20.0/wasmtime/struct.Store.html, instead).

Do you think if we remove the artifact cache these new features in validate will be a worthwhile maintenance burden?

@ktff
Copy link
Copy Markdown
Contributor Author

ktff commented Oct 17, 2020

I was already on the edge if this fix is worth it, so if the issue will be resolved on it's own later, yea, this isn't worthy of maintenance. I'll see if there is some lighter(but hackier) fix, if not, I think we can leave it as it is since it has specific conditions of validate + different access group + wasm transform.

@ktff
Copy link
Copy Markdown
Contributor Author

ktff commented Oct 17, 2020

I don't see a lighter way to fix this than this PR, so I'll add a description/reasoning in the issue and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: cli Anything related to Vector's CLI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants