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

Use pkl-gha for GitHub Action Workflows #47

Merged
merged 20 commits into from
Oct 10, 2024

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Aug 26, 2024

Hey there 👋

as a pkl-community you might be interested in using pkl "everywhere" 🙃
So I played over the weekened a bit and introduce pkl-gha (pkl for github actions) to this project.

I don't mind if gets rejected.
But I just thought as pkl "fanatics" I thought it make sense to use it here 😁

Maybe todos:

@jasongwartz
Copy link
Contributor

I love the idea!

I don't mind commit hooks as a developer convenience, but they're insufficient to guarantee consistency. Could you please add a new CI check that verifies that the generated YAML config is up-to-date with the Pkl source (like the TS dist one)?

.githooks/pre-commit Outdated Show resolved Hide resolved
.githooks/pre-commit Outdated Show resolved Hide resolved
@jasongwartz
Copy link
Contributor

We might also want to remove the dependabot config for GitHub Actions, since it would propose updates to the generated files, not the pkl sources: https://github.com/pkl-community/setup-pkl/blob/main/.github/dependabot.yml#L3

@StefMa StefMa force-pushed the pkl-gha branch 2 times, most recently from 597812c to d7247e4 Compare August 26, 2024 18:40
@StefMa
Copy link
Contributor Author

StefMa commented Aug 26, 2024

We might also want to remove the dependabot config for GitHub Actions, since it would propose updates to the generated files, not the pkl sources: https://github.com/pkl-community/setup-pkl/blob/main/.github/dependabot.yml#L3

Actually I wouldn't do that.
With that we would miss all the automatic updates by dependabot.

Since dependabot runs on GitHub actions we can add chain a new workflow with that.
As soon as dependebot updates an action, we can automaticallty run another action that updates the pkl file.
That should work ™️ but I haven't tried it (yet).

For now I would manually update the pkl files.

@StefMa StefMa changed the title use pkl-gha for GitHub Action Workflows Use pkl-gha for GitHub Action Workflows Aug 27, 2024
@jasongwartz
Copy link
Contributor

With that we would miss all the automatic updates by dependabot.

I'm not talking about disabling Dependabot altogether, just for the Actions part. I don't think it makes sense for it to open PRs to generated files.

@jasongwartz
Copy link
Contributor

Related, it would be helpful to have pkl render out the files as eg ci.gen.yaml or ci.generated.yaml to make it clear they are not to be edited, and to add a preamble that says "THIS IS GENERATED, DO NOT EDIT".

@StefMa
Copy link
Contributor Author

StefMa commented Aug 27, 2024

Related, it would be helpful to have pkl render out the files as eg ci.gen.yaml or ci.generated.yaml to make it clear they are not to be edited, and to add a preamble that says "THIS IS GENERATED, DO NOT EDIT".

Right now the generated yml files get the preamble:

# Do not modify!
# This file was generated from a template using https://github.com/StefMa/pkl-gha

Will also add the generated middle part to the files ✔️

Is this even possible with an custom renderer? 🤔
Can the pkl rendere define the output name? How?

@StefMa
Copy link
Contributor Author

StefMa commented Sep 27, 2024

Hey @jasongwartz

I wanted to push this to the finish line 🙃

This question is still open:
#47 (comment)
Would be grat if you can answer this.
Be note that I'm not that deep into the javascript/npm word.
I might need some guidance how to add this to the package.json or some clear instructions what is expected from your side.
Thanks!

Other than that, I also rereanged some things:

  • I moved the pkl-workflows into .github/pkl-workflows.
  • I moved the git-hook to a normal bash screen inside .github/pkl-workflows
    • I also executed this as part of the github action check (check-action.pkl)

Thats it I guess 🙃

Can't wait to get this thing merged 😇

@jasongwartz
Copy link
Contributor

Could you please incorporate the changes from #41 ?

["test-action"] {
strategy {
matrix {
["os"] = new {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this has been typed correctly in your base module, you shouldn't need = new {} to create a new list, the override syntax ["os"] { ... } should work. The only reason to do that typically with a Listing would be if there are default values in the parent object and you want to "start fresh" with a new Listing. Let me go check the typing in your parent module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://github.com/StefMa/pkl-gha/blob/main/GitHubAction.pkl#L592, I think there's extra complexity because matrix is typed as Listing<String|*Dynamic>. Can it really be a Dynamic object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be Dynamic.
See https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#example-using-a-multi-dimension-matrix

A variable configuration in a matrix can be an array of objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha - to me that looks more like it could be Mapping<String, String> - remember that Dynamic can contain properties, entries, AND elements, eg:

new {
  "an entry"
  prop1 = "value"
  ["entry one"] = "value2"
}

Note that this is actually not representable in JSON or YAML at all (below is from the REPL):

pkl> x = new {
>   "an entry"
>   prop1 = "value"
>   ["entry one"] = "value2"
> }
pkl> (new JsonRenderer {}).renderDocument(x)
–– Pkl Error ––
Cannot render object with both elements and properties/entries as JSON.
Object: new Dynamic { prop1 = "value"; ["entry one"] = "value2"; "an entry" }

But that's an aside, not important to this PR. Just info for you and your library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint!
Do you now how I could model this better/differently? 🤔

@jasongwartz
Copy link
Contributor

Thanks @StefMa ! Left a few other pointers.

Re:

I might need some guidance how to add this to the package.json or some clear instructions what is expected from your side.

In package.json, there is a field called "scripts" - each of these are little one-liners that can be run with npm run - that's where eg. npm run package is defined.

So could you please add a new script there called something like "gen:actions", which runs the one-liner from my comment on the current bash script? (The script won't be necessary after that) And then you can modify the "npm run package" script to add && npm run gen:actions (so that running npm run package does both 😄 )

@jasongwartz
Copy link
Contributor

Feel free to send me a message on Discord if you want a hand working through any of that! And sorry for the delay in getting back to you on this

@StefMa
Copy link
Contributor Author

StefMa commented Oct 10, 2024

@jasongwartz its ready for review again 🙃

Copy link
Contributor

@jasongwartz jasongwartz left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the contribution!

@jasongwartz jasongwartz merged commit 52a5818 into pkl-community:main Oct 10, 2024
8 checks passed
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