Skip to content
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

gotohelm: restore to working order #348

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

chrisseto
Copy link
Contributor

@chrisseto chrisseto commented Dec 6, 2024

Prior to this commit gotohelm failed to run post it's migration to this repository due to complications with LoadPackages and now being hosted in a new workspace layout.

The root of this issue stemmed from the AST rewrites being performed on the bootstrap package which would introduce a dependency from internal/bootstrap to helmette for functions like DictTest, Compact, and TypeTest.

This commit eliminates the helper functions and AST rewriting in favor of enhancing the transpiler itself. This allows the internal/bootstrap package to solely depend on the stdlib making it possible to load the package without any hijinks or dependency on cwd.

K8S-443

@chrisseto
Copy link
Contributor Author

@RafalKorepta this doesn't fix any issues in the charts directory itself. I managed to get gotohelm to run enough of the transpilation process that I think it's back to working order. Expect to see a fair amount of template diffs when we finally regenerate the charts.

@chrisseto
Copy link
Contributor Author

Ah this is probably in need of the licenseupdater change from #344

@chrisseto chrisseto force-pushed the chris/p/fix-gotohelm branch from 8fb242c to 43867ba Compare December 9, 2024 19:59
Prior to this commit gotohelm failed to run post it's migration to this
repository due to complications with `LoadPackages` and now being hosted in a
new workspace layout.

The root of this issue stemmed from the AST rewrites being performed on the
bootstrap package which would introduce a dependency from `internal/bootstrap`
to `helmette` for functions like `DictTest`, `Compact`, and `TypeTest`.

This commit eliminates the helper functions and AST rewriting in favor of
enhancing the transpiler itself. This allows the `internal/bootstrap` package
to solely depend on the stdlib making it possible to load the package without
any hijinks or dependency on cwd.
@chrisseto chrisseto force-pushed the chris/p/fix-gotohelm branch from 43867ba to aa9f859 Compare December 10, 2024 18:32
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisseto
Copy link
Contributor Author

So this does get gotohelm running again but there's a regression hiding around that I just uncovered. It seems that something in this PR make gotohelm drop int64 casts 🤔

@chrisseto
Copy link
Contributor Author

Think I got a lead. It's the alias type messing with maybeCast.

@chrisseto
Copy link
Contributor Author

I think test failures are due to the retirement of the vectorized repo 😓

// the identifiers on the LHS and position of the AssignStmt to get something that is unique,
// deterministic, and mildly human readable.
// foo, ok := ... -> $_123_foo_ok := ...
intermediate := &Ident{Name: fmt.Sprintf("_%d", stmt.Pos())}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why, but the stmt.Pos() is not stable. When this would be added to CI it will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an example failure I could see?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just re-run

go test ./pkg/gotohelm -v 

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

I will work on making transpileMVAssignStmt stable in the next PR

@RafalKorepta RafalKorepta merged commit 26e8da7 into main Dec 11, 2024
4 of 5 checks passed
@chrisseto chrisseto deleted the chris/p/fix-gotohelm branch December 11, 2024 15:07
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.

2 participants