-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add support for pkl files #331
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
Open
akhil-rasheed
wants to merge
20
commits into
Boeing:main
Choose a base branch
from
akhil-rasheed:feat/pkl-validation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+195
−5
Open
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
395392c
feat: add support for pkl files
olunusib b706fe9
feat: skip pkl files when binary isn't in PATH
olunusib 80c436b
Merge branch 'main' into feat/pkl-validation
olunusib 1fb119b
chore: use the new action tag in ref comment
olunusib 2e01953
fix(test): resolve data race in pkl validation test
df3e3de
fix: move PKL to alphabetically correct spot in list of supported con…
e33c522
test: add fleshed out example for good.pkl
4f54b22
Merge branch 'main' into feat/pkl-validation
akhil-rasheed a3f1ca6
fix(test): repair test suite after pkl validator refactor
1b7e7cd
fix: correct build and pkl test fixture
e59a61c
fix: remove whitespace in workflow file
309847e
feat(pkl): Improve test coverage for pkl validator
8ffa41d
fix(test): Prevent race conditions in pkl validator tests
akhil-rasheed 158c3c6
refactor: rename ErrSkipped to ErrPklSkipped
akhil-rasheed 7fe75c1
refactor: improve pkl binary detection
akhil-rasheed 5a7e6c0
revert: add back validator to .gitignore
akhil-rasheed b2e4e29
Merge branch 'main' into feat/pkl-validation
akhil-rasheed 29461bc
fix: remove whitespace
akhil-rasheed 8398985
fix: remove whitespace
akhil-rasheed 306e190
refactor(suggestion): mark unused parameters
akhil-rasheed File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package validator | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"os/exec" | ||
|
||
"github.com/apple/pkl-go/pkl" | ||
) | ||
|
||
var ( | ||
// ErrPklSkipped is returned when a validation is skipped due to a missing dependency. | ||
ErrPklSkipped = errors.New("validation skipped") | ||
) | ||
Check failure on line 15 in pkg/validator/pkl.go
|
||
// PklValidator is used to validate a byte slice that is intended to represent a | ||
// PKL file. | ||
type PklValidator struct { | ||
evaluatorFactory func(context.Context, ...func(*pkl.EvaluatorOptions)) (pkl.Evaluator, error) | ||
} | ||
|
||
// Validate attempts to evaluate the provided byte slice as a PKL file. | ||
// If the 'pkl' binary is not found, it returns ErrSkipped. | ||
func (v PklValidator) Validate(b []byte) (bool, error) { | ||
ctx := context.Background() | ||
|
||
// Convert the byte slice to a ModuleSource using TextSource | ||
source := pkl.TextSource(string(b)) | ||
|
||
evaluatorFactory := v.evaluatorFactory | ||
if evaluatorFactory == nil { | ||
evaluatorFactory = pkl.NewEvaluator | ||
} | ||
|
||
evaluator, err := evaluatorFactory(ctx, pkl.PreconfiguredOptions) | ||
if err != nil { | ||
// If the error is that the pkl binary was not found, return ErrPklSkipped. | ||
var execErr *exec.Error | ||
if errors.As(err, &execErr) && execErr.Err == exec.ErrNotFound { | ||
Check failure on line 39 in pkg/validator/pkl.go
|
||
return false, ErrPklSkipped | ||
} | ||
return false, fmt.Errorf("failed to create evaluator: %w", err) | ||
} | ||
|
||
_, err = evaluator.EvaluateExpressionRaw(ctx, source, "") | ||
if err != nil { | ||
return false, fmt.Errorf("failed to evaluate module: %w", err) | ||
} | ||
|
||
return true, nil | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
|
||
|
||
name = "PhoenixWebApp" | ||
|
||
package { | ||
name = "phoenix" | ||
version = "2.1.0" | ||
authors = List("Phoenix Engineering <[email protected]>") | ||
} | ||
|
||
database { | ||
host = "db.phoenix.internal" | ||
port = 5432 | ||
username = "phoenix_user" | ||
poolSize = 20 | ||
sslMode = "require" | ||
} | ||
|
||
server { | ||
host = "0.0.0.0" | ||
port = 8080 | ||
logLevel = "info" // "debug", "info", "warn", "error" | ||
corsOrigins = List("https://phoenix.example", "https://app.phoenix.example") | ||
} | ||
|
||
features { | ||
newSignupFlow = true | ||
apiRateLimiting = true | ||
dashboardAnalytics = false | ||
} | ||
|
||
// --- Service Replica Configuration --- | ||
local class Replica { | ||
region: "us-east-1" | "us-west-2" | "eu-central-1" | ||
instanceType: String | ||
count: Int | ||
} | ||
|
||
replicas = List( | ||
new Replica { | ||
region = "us-east-1" | ||
instanceType = "t3.medium" | ||
count = 3 | ||
}, | ||
new Replica { | ||
region = "us-west-2" | ||
instanceType = "t3.medium" | ||
count = 2 | ||
} | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"invalid" |
Oops, something went wrong.
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.
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.
The unit tests shouldn't rely on PKL being present - that should be mocked so the tests are deterministic
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.
Ok we can do this. But I'll have to extract out the pkl tests to their own function, deviating from the existing pattern of putting tuples in the testData array
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.
That's fine - it's the first time we have to rely on the presence of a local binary
Uh oh!
There was an error while loading. Please reload this page.
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.
@akhil-rasheed Looking at the PKL Go documentation, I do not think the binary is required anymore. The PKL binary is only required when using code generation and it looks like you can load PKL into a struct without using Code Generation: https://pkl-lang.org/go/current/evaluation.html#without-code-generation. This may only apply to a pre-defined struct though which wouldn't work.
Please take a look and see if this is a viable path. This would greatly simplify the implementation.