Skip to content

Conversation

@vkumbhar94
Copy link
Contributor

@vkumbhar94 vkumbhar94 commented Mar 5, 2025

Currently pkgxdev is failing to autodetect skaffold when skaffold.yaml is having multiple documents with following error:

╰─ z skaf                                                                                                            ─╯
-deno.land -deno^2 -nodejs.org
error: Uncaught (in promise) SyntaxError: Found more than 1 document in the stream: expected a single document
    throw new SyntaxError(
          ^
    at parse (https://jsr.io/@std/yaml/1.0.5/parse.ts:87:11)
    at Path.readYAML (https://raw.githubusercontent.com/pkgxdev/libpkgx/refs/tags/v0.20.1/src/utils/Path.ts:405:14)
    at eventLoopTick (ext:core/01_core.js:177:7)
    at async skaffold_yaml (file:///Users/vkumbhar94/.pkgx/pkgx.sh/dev/v1.5.0/share/pkgx/dev/src/sniff.ts:263:18)
    at async default (file:///Users/vkumbhar94/.pkgx/pkgx.sh/dev/v1.5.0/share/pkgx/dev/src/sniff.ts:61:11)
    at async default (file:///Users/vkumbhar94/.pkgx/pkgx.sh/dev/v1.5.0/share/pkgx/dev/src/dump.ts:6:17)
    at async file:///Users/vkumbhar94/.pkgx/pkgx.sh/dev/v1.5.0/share/pkgx/dev/app.ts:48:7
Caused by: "skaffold-modules/skaffold.yaml"

Once this util function is available to read multi-yaml file, I will raise PR to pkgx.

issue ticket: #82

@coveralls
Copy link

coveralls commented Mar 5, 2025

Pull Request Test Coverage Report for Build 13682642985

Details

  • 14 of 14 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 82.859%

Totals Coverage Status
Change from base Build 12831381962: 0.3%
Covered Lines: 2471
Relevant Lines: 2914

💛 - Coveralls

@vkumbhar94 vkumbhar94 force-pushed the dev-vk-readyamlall branch 4 times, most recently from e28eed8 to 89799d1 Compare March 5, 2025 18:05
@vkumbhar94
Copy link
Contributor Author

@mxcl I would request you to review the PR.

@vkumbhar94 vkumbhar94 force-pushed the dev-vk-readyamlall branch from 89799d1 to 4642807 Compare March 5, 2025 18:11
@vkumbhar94
Copy link
Contributor Author

@jhheider, can you pls take a look? I am thinking to spend weekend to fix this in pkgx repo.

Copy link
Contributor

@jhheider jhheider left a comment

Choose a reason for hiding this comment

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

this looks reasonable as it goes. two notes:

  • this is for pkgx v1; pkgx v2 is pure rust (pkgxdev/pkgx)
  • some determination of what should be done with a yaml array needs to be made. the simplest is probably just the read the first one and stop. to what end does skaffold.dev multiple documents in its file?

@vkumbhar94
Copy link
Contributor Author

vkumbhar94 commented Mar 9, 2025

this looks reasonable as it goes. two notes:

  • this is for pkgx v1; pkgx v2 is pure rust (pkgxdev/pkgx)

Now with pkgx v2, we should need it at https://github.com/pkgxdev/dev/blob/main/src/sniff.ts#L263 , isn't it?

  • some determination of what should be done with a yaml array needs to be made. the simplest is probably just the read the first one and stop. to what end does skaffold.dev multiple documents in its file?

Say, if we run skaffold run command in a directory where skaffold.yaml is written, it runs all the documents defined inside it. Therefore we need to iterate over all those and add deps accordingly.

@jhheider
Copy link
Contributor

jhheider commented Mar 9, 2025

that looks correct. got it.

@vkumbhar94
Copy link
Contributor Author

Thanks @jhheider for reviewing it,pls proceed to merge.

@vkumbhar94
Copy link
Contributor Author

Gentle reminder @jhheider if you missed to merge.

@jhheider jhheider requested a review from mxcl March 15, 2025 07:26
@mxcl
Copy link
Member

mxcl commented Mar 29, 2025

Hey sorry I missed this.

Are you still using pkgx^1? In which case this will fix it but we'd have to release a new v1 release.

If for v2 and the separate dev tool then I’m not sure dev uses this particular function from libpkgx.

@mxcl mxcl merged commit 65b6528 into pkgxdev:main Mar 29, 2025
8 checks passed
@vkumbhar94
Copy link
Contributor Author

Hey sorry I missed this.

Are you still using pkgx^1? In which case this will fix it but we'd have to release a new v1 release.

If for v2 and the separate dev tool then I’m not sure dev uses this particular function from libpkgx.

No, I've switched to pkgx^2.

I believe the separate dev tool (sniff.ts#L263) uses libpkgx. If that’s incorrect, could you please point me to the right location to address the multi-YAML Skaffold issue with pkgx^2?

@vkumbhar94
Copy link
Contributor Author

Hi @mxcl @jhheider , I have opened a follow-up PR on dev tool: pkgxdev/dev#38 . Please have a look.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants