feat: add support for pkl files #164

olunusib · 2024-08-21T03:07:51Z

Adds support for PKL files (https://pkl-lang.org/). Note that pkl-go requires Go 1.22.1. This might be considered a breaking change for users of this tool who are on earlier Go versions.

Closes #141.

olunusib · 2024-08-21T22:33:14Z

@kehoecj @ccoVeille The CI is failing due to this issue with pkl-go. Basically, pkl needs to be installed for the tests to pass.

We could address this by adding a step in the workflow to install pkl before running the tests and including a note in the documentation to inform users that pkl must be installed to use this tool with pkl files. Should note that pkl comes with a built-in evaluator. Let me know how you want to proceed.

kehoecj · 2024-08-22T16:25:57Z

@kehoecj @ccoVeille The CI is failing due to this issue with pkl-go. Basically, pkl needs to be installed for the tests to pass.

We could address this by adding a step in the workflow to install pkl before running the tests and including a note in the documentation to inform users that pkl must be installed to use this tool with pkl files. Should note that pkl comes with a built-in evaluator. Let me know how you want to proceed.

I'm not sure I want to introduce a binary dependency for the validator for a single config type - especially because our release system relies on downloading statically-linked binaries or running go install rather than through package managers like RPMs/Debs, Homebrew, Chocolately, etc. We have started down that path with an AUR for arch linux and I'd like to keep adding packages and publishing to package managers but until we do I don't think we can support binary dependencies without negatively impacting the current installation workflow for existing users.

ccoVeille · 2024-08-22T16:33:35Z

I agree. Is there another pkl validation only in Go?

ccoVeille · 2024-08-22T16:40:30Z

But wait, do you need the code generation?

As documented it's the only use case when the binary is required

https://pkl-lang.org/go/current/quickstart.html#1-install-dependencies

olunusib · 2024-08-23T04:01:22Z

@kehoecj @ccoVeille The CI is failing due to this issue with pkl-go. Basically, pkl needs to be installed for the tests to pass.
We could address this by adding a step in the workflow to install pkl before running the tests and including a note in the documentation to inform users that pkl must be installed to use this tool with pkl files. Should note that pkl comes with a built-in evaluator. Let me know how you want to proceed.

I'm not sure I want to introduce a binary dependency for the validator for a single config type - especially because our release system relies on downloading statically-linked binaries or running go install rather than through package managers like RPMs/Debs, Homebrew, Chocolately, etc. We have started down that path with an AUR for arch linux and I'd like to keep adding packages and publishing to package managers but until we do I don't think we can support binary dependencies without negatively impacting the current installation workflow for existing users.

Agreed

olunusib · 2024-08-23T04:02:58Z

But wait, do you need the code generation?

As documented it's the only use case when the binary is required

https://pkl-lang.org/go/current/quickstart.html#1-install-dependencies

There are 2 binaries:

pkl-gen-go: for code generation (not applicable in this case)
pkl always needed

ccoVeille · 2024-08-24T10:41:52Z

Then pkl format is a pain

This project could help

https://github.com/Drafteame/pkl-linter

But for now, it only supports CLI. But we could test the tool and ask developer (or help him) to provide a library

kehoecj · 2024-08-27T13:22:52Z

@olunusib @ccoVeille Trying to think of a way to keep this work without negatively impacting the installation experience for users who have no need for PKL and don't have the binary installed. What if PKL was opt-in only (at least for now) where if go can't find the pkl executable on the path then it won't try to lint it? If it finds a .pkl file during validation but PKL is not installed, it will give a warning saying something like "the following PKL files were found but not linted because the PKL binary was not found on the path".

For the Github Action and AUR/Scoop/WinGet packages I can add PKL as a dependency so that shouldn't be a problem for that type of distribution.

What do you think?

ccoVeille · 2024-08-27T20:28:27Z

I like your suggestion

kehoecj · 2024-08-30T16:50:30Z

It likes your suggestion

@olunusib What do you think? Would you want to take a shot at making PKL optional as described above?

olunusib · 2024-08-31T00:49:42Z

Sounds like a good plan @kehoecj. I'll find some time this weekend to look into it

olunusib · 2024-09-02T21:18:44Z

@kehoecj @ccoVeille this should be ready for review. I'd like to see the workflow run at least.

ccoVeille · 2024-09-03T07:09:45Z

pkg/cli/cli_test.go

+	// Save the original function before mocking and restore it after the test
+	originalIsPklBinaryPresent := isPklBinaryPresent
+	isPklBinaryPresent = func() bool {
+		return false
+	}
+	defer func() { isPklBinaryPresent = originalIsPklBinaryPresent }()


I'm afraid this could lead to data race, when other tests are running

Using a lock mechanism to solve this would be over complicated as it would be applied even with the CLI

Tests within the same package run sequentially, so there shouldn't be a risk of a data race here. Lmk if that's not the case.

If we decide to make these parallel later, I think adding locking wouldn't be a bad idea. Can you explain what you mean by "it would be applied even with the CLI"?

The locking I was suggesting would have been a mutex.Read/Write + defer unlock

to use it in the tests, I think it would require to use it in the code too, so when trying to check if isPklBinaryPresent here, when using it in the CLI, the lock would have to be present

The test are ran sequentially, but using a -race help you to spot such issues.

ccoVeille · 2024-09-03T07:13:37Z

pkg/cli/cli.go

+		// Pkl validation requires the 'pkl' binary to be present
+		if fileToValidate.FileType.Name == "pkl" {
+			if !isPklBinaryPresent() {
+				fmt.Printf("Warning: 'pkl' binary not found, file %s will be ignored.\n", fileToValidate.Path)
+				continue
+			}
+		}
+


I would move this in the pkl validator

Same response as below

Let's go back to the reason I raised the point of doing something else that what is in this PR

I see a problem in having a loop, then adding a dirty if hack in the loop.

The validator should be in charge of validating, it's not the loop that should do a big if.

ccoVeille · 2024-09-03T07:23:55Z

pkg/cli/cli.go

+// Check if the 'pkl' binary is present in the PATH
+var isPklBinaryPresent = func() bool {
+	_, err := exec.LookPath("pkl")
+	return err == nil
+}


I would move this as a method of pkl validator

This would help you to validate it's behavior, abdband would prevent race in tests

I briefly contemplated this but decided against it for 2 reasons:

The intended behavior is to skip the files entirely. If we handle this within the pkl validator, we'd have to return a true or false value, which implies that we've performed the validation and determined whether the file is good or bad. This feels like a breach of contract IMO and it goes against our intention of skipping the files altogether.

We would lose the ability to log the exact files that were skipped since validate only takes the content of the files, not the file paths

I agree with the second point, it would need to rewrite a part of the code

About the idea of perf, I don't mind here. If there is a pkl file, going to the pkl code to see if the file is valid, would require to validate its content. So it would be slow. Of course, when the pkl binary is not present, the code might have to go through the code and finally do not execute the code

Please note you could consider simulating the binary presence, error, absence by calling arbitrary binary name:

true, always return 0

false, always return 1

a random string, the binary will be absent

It would be better than overloading the isPklBinaryPresent by something else

I'm unsure it's a good idea, but why not, it may help

I agree with @ccoVeille and feel like the validator should be able to validate if its enabled or not rather than having the check in the CLI. The validator will see the file, realize that it is not set up properly (no PKL) and then skip it and provide that feedback to the report.

kehoecj

Aadd the PKL binary to the Dockerfile. Instructions
Update the README to point to the PKL Installation Instructions so users know how to install it
If the PKL files are skipped because the PKL binary is not available I'd like to have it show in the report summary as skipped:
```
  "summary": {
      "passed": 37,
      "failed": 12,
      "skipped": 2
  }
```

kehoecj · 2024-10-07T13:54:54Z

cmd/validator/validator.go

@@ -2,7 +2,7 @@
 Validator recursively scans a directory to search for configuration files and
 validates them using the go package for each configuration type.

-Currently Apple PList XML, CSV, HCL, HOCON, INI, JSON, Properties, TOML, XML, and YAML.
+Currently Apple PList XML, PKL, CSV, HCL, HOCON, INI, JSON, Properties, TOML, XML, and YAML.


Alphabetize please

kehoecj · 2024-10-07T14:04:50Z

pkg/validator/pkl.go

+	ctx := context.Background()
+
+	// Convert the byte slice to a ModuleSource using TextSource
+	source := pkl.TextSource(string(b))


Are there conditions where pkl.TextSource will return an err? Do we need to handle the err?

kehoecj · 2024-10-07T14:06:17Z

test/fixtures/good.pkl

@@ -0,0 +1,7 @@
+name = "Swallow"


Would prefer a more complete example

kehoecj · 2024-10-21T17:44:13Z

@olunusib Are you waiting on me for anything on this?

feat: add support for pkl files

395392c

kehoecj added the waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers label Aug 21, 2024

ccoVeille approved these changes Aug 21, 2024

View reviewed changes

kehoecj removed the waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers label Aug 26, 2024

kehoecj mentioned this pull request Aug 27, 2024

Add PKL as a runtime dependency kehoecj/config-file-validator-aur#1

Open

ccoVeille mentioned this pull request Aug 31, 2024

Could you provide a library to validate a PKI file? Drafteame/pkl-linter#4

Open

olunusib added 3 commits September 1, 2024 19:58

feat: skip pkl files when binary isn't in PATH

b706fe9

Merge branch 'main' into feat/pkl-validation

80c436b

chore: use the new action tag in ref comment

1fb119b

ccoVeille reviewed Sep 3, 2024

View reviewed changes

kehoecj added the waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers label Sep 3, 2024

kehoecj added the hacktoberfest 🎃 Hacktoberfest 2024 label Oct 2, 2024

kehoecj requested changes Oct 7, 2024

View reviewed changes

kehoecj reviewed Oct 7, 2024

View reviewed changes

test/fixtures/good.pkl

@@ -0,0 +1,7 @@

name = "Swallow"

Copy link

Collaborator

kehoecj Oct 7, 2024

Choose a reason for hiding this comment

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

Would prefer a more complete example

kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for pkl files #164

feat: add support for pkl files #164

olunusib commented Aug 21, 2024

olunusib commented Aug 21, 2024

kehoecj commented Aug 22, 2024

ccoVeille commented Aug 22, 2024

ccoVeille commented Aug 22, 2024

olunusib commented Aug 23, 2024

olunusib commented Aug 23, 2024

ccoVeille commented Aug 24, 2024

kehoecj commented Aug 27, 2024 •

edited

Loading

ccoVeille commented Aug 27, 2024 •

edited

Loading

kehoecj commented Aug 30, 2024

olunusib commented Aug 31, 2024

olunusib commented Sep 2, 2024

ccoVeille Sep 3, 2024

olunusib Sep 4, 2024

ccoVeille Sep 8, 2024

ccoVeille Sep 3, 2024

olunusib Sep 4, 2024

ccoVeille Sep 8, 2024

ccoVeille Sep 3, 2024

olunusib Sep 4, 2024 •

edited

Loading

ccoVeille Sep 8, 2024

kehoecj Oct 7, 2024

kehoecj left a comment •

edited

Loading

kehoecj Oct 7, 2024

kehoecj Oct 7, 2024

kehoecj Oct 7, 2024

kehoecj commented Oct 21, 2024

feat: add support for pkl files #164

Are you sure you want to change the base?

feat: add support for pkl files #164

Conversation

olunusib commented Aug 21, 2024

olunusib commented Aug 21, 2024

kehoecj commented Aug 22, 2024

ccoVeille commented Aug 22, 2024

ccoVeille commented Aug 22, 2024

olunusib commented Aug 23, 2024

olunusib commented Aug 23, 2024

ccoVeille commented Aug 24, 2024

kehoecj commented Aug 27, 2024 • edited Loading

ccoVeille commented Aug 27, 2024 • edited Loading

kehoecj commented Aug 30, 2024

olunusib commented Aug 31, 2024

olunusib commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olunusib Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kehoecj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kehoecj commented Oct 21, 2024

kehoecj commented Aug 27, 2024 •

edited

Loading

ccoVeille commented Aug 27, 2024 •

edited

Loading

olunusib Sep 4, 2024 •

edited

Loading

kehoecj left a comment •

edited

Loading