chore: fix circular dependencies related to assets.ts #1357
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1357 +/- ##
==========================================
- Coverage 79.57% 75.21% -4.36%
==========================================
Files 39 49 +10
Lines 1816 2255 +439
Branches 396 444 +48
==========================================
+ Hits 1445 1696 +251
- Misses 342 520 +178
- Partials 29 39 +10
|
src/cli/build.ts
Outdated
| } else { | ||
| zarf = assets.zarfYaml(yamlFile); | ||
| zarf = await assets.zarfYaml(yamlFile); |
There was a problem hiding this comment.
| zarf = await assets.zarfYaml(yamlFile); | |
| zarf = await assets.zarfYaml(yamlFile); |
Why is this async?
There was a problem hiding this comment.
The return value is a Promise so the await is needed or you get:
Type 'Promise<string>' is not assignable to type 'string'.ts(2322)
There was a problem hiding this comment.
Fixed these by making the functions in assetsDeployer.ts no be async.
src/cli/build.ts
Outdated
| @@ -180,9 +176,9 @@ export default function (program: RootCmd) { | |||
|
|
|||
| let zarf = ""; | |||
| if (opts.zarf === "chart") { | |||
| zarf = assets.zarfYamlChart(chartPath); | |||
| zarf = await assets.zarfYamlChart(chartPath); | |||
There was a problem hiding this comment.
this should not be async
There was a problem hiding this comment.
Fixed these by making the functions in assetsDeployer.ts no be async.
src/cli/build.ts
Outdated
| @@ -111,7 +103,9 @@ export default function (program: RootCmd) { | |||
| // only actually build/push if there are files to include | |||
| if (includedFiles.length > 0) { | |||
| await createDockerfile(cfg.pepr.peprVersion, cfg.description, includedFiles); | |||
| execSync(`docker build --tag ${image} -f Dockerfile.controller .`, { stdio: "inherit" }); | |||
| execSync(`docker build --tag ${image} -f Dockerfile.controller .`, { | |||
There was a problem hiding this comment.
What happened to the customImage flag?
There was a problem hiding this comment.
I removed it by accident when I was fixes merge conflicts yesterday.
src/lib/assets/assetsDeployer.ts
Outdated
| return allYaml(this.assetsConfig, imagePullSecret); | ||
| } | ||
|
|
||
| async zarfYaml(path: string) { |
There was a problem hiding this comment.
doesn't this just return some json?
src/lib/assets/assetsDeployer.ts
Outdated
| return zarfYaml(this.assetsConfig, path); | ||
| } | ||
|
|
||
| async zarfYamlChart(path: string) { |
There was a problem hiding this comment.
doesn't this just return some json? not async needed unless i am missing something
|
this also breaks customImage and image in the build cli command |
Description
The assets modules have several circular dependencies between them. These should be fixed.
...
End to End Test:
Related Issue
Fixes #1340
Relates to #
Type of change
Checklist before merging