https://kotlinlang.org logo
Join Slack
Powered by
since have been thinking about ways to make sure t...
# github-workflows-kt
n
since have been thinking about ways to make sure that you are using the latest version of a given action.. how feasible would it be to do this at runtime of the script ? when compiling a workflow into yaml you have the action and version tag for it.. (dependencies to actions that are added in the script but not used in any workflow would be ignored, but i think thats okay as far as limitations go) could it log to stderr when a new version is found for any of the actions ? or should it just print out a ready to copy-paste block of depdnency annotations for you to copy in ? this would not be as ideal as a external tool to automatically add comments but it would just show up in either your console or on github in the log (assuming you do use consistency checks) and i hope it would not add a lot of extra runtime i assume we can do github api calls to check for available tags on actions and use the 
GITHUB_TOKEN
when present in env like so
Copy code
env:
    GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
as show in githubs own example
p
I'd rather not bundle this functionality in github-workflows-kt, but you could: • create a separate Kotlin logic that would accept a Workflow object that is returned form the 
workflow
DSL builder, traverse to the steps and extract the versions, like
Copy code
val workflow = workflow(...) { }
workflow.checkIfNewerVersionsExist()
workflow.writeToFile()
• parse the resulting GitHub workflow YAML and extract versions from there
n
very helpful link, i will have to dig into this in detail to see what i can find i might do this.. hate having to publish my own library for this though
p
ok, we can create a separate module in github-workflows-kt monorepo and publish it as a separate lib, like here: https://github.com/typesafegithub/github-workflows-kt/blob/b49d8c93d35d25da4ef5641bb3b0e38bd8cc66be/.github/workflows/release.main.kts#L49
❤️ 1
like: • module: 
action-versions-updates
• a simple extension function 
workflow.printActionVersionUpdates()
names TBD
n
i think writing markdown to the GITHUB_STEP_SUMMARY is probably the most reasonable reporting tool next to plain out println()
p
Ack, will take a look likely tomorrow
n
for now i am fighting with jitpack to hopefully let me load this without needing a release but its painful
and it seems to fail hard.. jitpack is not really capable of dealing with neer gradle stuff anymore huh ? https://jitpack.io/com/github/NikkyAI/github-workflows-kt/dev-4115e4c3d8-1/build.log
p
the first error I see is 
Unresolved reference: http
- for some reason, JitPack uses Java 8 (
openjdk version "1.8.0_252"
), and the HTTP client was added in 11. We have the toolchain version set to 11 here, but I think it only specifies which JDK should be used to build the actual app's logic, not the 
buildSrc
. So you need to tell JitPack to use a specific version of JDK to run Gradle in the first place
alternatively, you can temporarily (on your branch/fork) get rid of the 
AwaitMavenCentralDeployTask.kt
file which contains Java 11-specific API since it's needed in the release pipeline only
n
yeah, but then jitpack is also not doing the best job of anything that's multi module.. and expects specific plugins to use publishToMavenLocal some day.. but I don't feel like doing buildscript surgeries today
👌 1
p
n
was just about to send you the link.. some formatting.. maybe have the title say 
available updates
and such but .. minor details
also... i managed to wrangle jitpack into mostly working.. annoyingly jitpack does not have jdk17 bundled.. and the sdkmana install script fails randomly
other weirdness includes platform() bom dependencies did not work when jitpack was involved.. seems to be a somewhat known issue.. luckily all we need to depend on is ktor
runtime on that was horrible.. but thats all jitpack.. locally this goes way faster
well it seems like using jitpack for hosting artifacts is just ... as unreliable as you could image.. i have seen all sorts of errors.. uncomplete git clones (it did not detect a git repo and cancelled the build), failed jdk downloads... not running the task and then complaining that there is no output artifacts
anyways.. i fixed the issue with 
$
in the urls in the markdown report i think as far as i can tell this is ready for a release, in the future we can iterate on formatting easier once the base functionality is somewhere in a maven package
p
thanks, I'll review later! BTW, I'm wondering if it should return both major and specific versions, instead of either one depending on which one is present in the code
n
i considered that.. but i wanted "something" to work first once we have a first release we can iterate on this so much quicker by copying 2 functions into the workflow script and messing with it
p
got it, ok, let's take it iteratively - I'll review with that in mind
n
for iterating on what gets written into the summary, you can just run the script locally with the env variable set as well
i expect you probably want the PR a bit cleaned up.. we can remove the jitpack specific workarounds in the buildscript if you want too, it did not end up being too.. useful
p
I'll definitely point out things to clean up, so you can be proactive and do a self-review 😄
n
will look forward to the review.. for now i need to prepare to VJ this evening and do a few chores before they pile up
p
sure, thanks for working on it! will get back to you
👍 1
I see a lot of duplication in this PR, code copied from other modules. We'll need to sort it out
n
yep, it might make sense to move some of that into a shared module
p
I'll try to extract some of these things to 
shared-internal
so that your PR is simpler
n
can we allow for a nullable github token? the api can be called without auth and that would allow it to run locally without additional setup
p
Oh, does it work without the token? Or maybe there's some limit on the number of calls. Anyway, yeah - feel free to go this way, and we'll discuss in the PR if needed
n
as long as you don't hit the api rate limit which is fairly low for unauthenticated requests.. but it does work
👍 1
actually.. given how quickly i ran into the ratelimit myself.. lets just have people set up tokens.. its not that much effort
👍 1
I moved away from JitPack in favor of Maven snapshots
can i use those in .main.kts scripts ? do you have a example ?
i managed to make that somewhat work.. but it runs into NoClassDef errors again.. on github too https://github.com/NikkyAI/github-workflows-kt/actions/runs/8766910432/job/24059583389
p
To use maven snapshots, you'd have to publish them from your fork. You're probably better using JitPack or maven local
n
works locally ... sometimes... enough to make it build the yaml... (although idea is eternally confused and highlighting/code completelion is completely gone)
i am not 100% sure if this is because of the BOMs .. or if removing them just invalidates the cache because the dependency graph changed..
p
But the BOMs are in a regular Gradle project, not in the script, right? It should work
n
i think the kotlin scripting dependency resolution is just.. not very good
yeah i think i know what triggers this.. when using the BOM the dependency in the .pom file is 
ktor-client-core
without using a BOM it is 
ktor-client-core-jvm
only the 
-jvm
variant actually contains the classes we need.. and scripting is not magically looking this up
or.. maybe this is ONLY a issue in 
publishToMavenlocal
who knows... the gradle specific hacks that kotlin uses for its multiplatform dependneies do not work atm however and ktor dependencies are
p
Oh, looks like another bug in scripting! I really, strongly encourage you to report it, I'll happily include it in my little open letter to JB regarding fixing multiple issues in scripting
n
posted in #scripting for now.. maybe there is a magic fix someone knows
👍 1
p
@Nikky LMK once the PR is again ready to review (change from "draft" to "ready to review")
n
https://github.com/NikkyAI/github-workflows-kt/actions/runs/8783162644#summary-24098740370 line numbers work now, 
/
is correctly replaced by 
__
, there was a issue with actions that are in folders like 
gradle/actions/setup-gradle
i made the logging detect the environment and only print the github specific stuff then, formatting of normal terminal output still needs improvement and feedback maybe we want to move logging to shared-internal so it can be reused? or even into the main librry .. might be useful in logic steps
p
thanks, will TAL!
n
some other workflows regenerating the yaml causes it to error, so maybe now would be a good time to integrate the pre validation steps ?
p
what do you mean by pre-validation steps?
n
we probably need to add an extra step to the consistency job to load the artifact to .m2 prior to compiling the script. This step would exist only for the purpose of the integration tests, so we could add a generic extension point to the library to customize the consistency check job
https://kotlinlang.slack.com/archives/C02UUATR7RC/p1713768525002489?thread_ts=1713701263.135969&cid=C02UUATR7RC
p
"Build Kotlin scripts" and "Run consistency check on all GitHub workflows" fail in your PR because you still have the test workflow .github/workflows/demo_updates.main.kts. I think we should remove it prior to merging, as discussed
LMK if you have something else in mind
n
we can do that too
i deleted the script and yaml in the PR, as far as i am concerned this should be ready for review again
✅ 1
p
thanks, will review today or tomorrow
shared some comments, let's resolve them first before going forward
n
should we aim for a api surface that exposes some of the underlying structures to offer users some customization or go for the smallest api surface possible ?
p
I definitely opt for option 2: preparing for future possible use cases will cost us more. The more API we expose now, the more likely it is to introduce breaking changes in the future, and the less freedom we have with refactoring. I recommend having a simple API: a single extension function on 
Workflow
with minimal API, and that prints something to the job's summary. If some requests appear to provide more customization, we'll consider it then. Even if you are willing to contribute to this module in the future, I'll technically own this code 🙂
n
okay, then i will make everything i can internal
🙌 1
and done, api surface is a lot smaller now
p
ok, we're close to getting it merged 🙂 shared probably the last round of comments, please also take a look at the past comments, not all of them are responded to/applied
n
i think i addressed all the comments? feel free to review
p
ok, I will probably later today, thanks! do you think it makes sense to make the end-to-end test one last time after the recent changes in the PR?
n
one last change we might need is passing the GITHUB_TOKEN into environment on the validation step
end to end test.. i can do that on a different branch without affecting the PR
p
> one last changew we might need is passing the GITHUB_TOKEN into environment on the validation step right, good spot! actually I'm wondering if it should run in the consistency check job. I'm thinking of something like:
Copy code
if (args[0] == "check-updates") {
    workflow.checkForUpdates()
}
then the consistency check job would remain fairly quick, and one could have a separate step that checks the updates. WDYT?
I'm open to other ways of controlling if the update checking logic should fire or not
n
having a separate step compile the script again, just with a special argument to run the version checks does not really give you any benefits here i think? the github summary might be messed up if its running in parallel with something else writing to it and runnign it locally (with provided token) would also be slightly more of a hassle and i am not sure how much boilerplate that would require in the scripts, we cannot access args outside of the script without at least passing it along
i think we should provide sample code for people to use it in that way.. but it should not be restricted to that
p
the main problem is that right now, the scripts are focused on just producing the YAML, so the script's runtime consists of: • compiling the script, if not already in the cache • starting up the JVM • executing very fast logic of producing the YAML once we enforce the users to start adding the extra call to check the versions to the script, it can make the runtime longer by even a minute or so
sure, people should have some kind of freedom how to use it, but I'm wondering if we should provide a way to pass the GITHUB_TOKEN to the consistency check job. Making this change not behind some feature flag would cause YAML regeneration for all customers, which would be a breaking change
would a flag like 
setGithubTokenInConsistencyCheckJob
help here? 😛
we could make it the default in v2 of the lib
n
we already have a 
yamlConsistencyJobCondition
can we add a argument for a env ? or pass it into 
writeYaml
?
i would avoid making a flag for a feature in a different module.. or naming it after that feature, if possible keep things generic
p
yamlConsistencyJobEnv: Map<String, String>
? LGTM
if you'd like to add it, let's make it a separate PR
n
since the yaml consistency job runs the script.. passing in a custom env might be helpful for other cases as well where people were forced to diable the consistency check or drop the idea before
👍 1
imho the parameters for the consistency check should all be grouped together.. rn one is in the workflow and the other in generateYaml.. i will add this new parameter next to 
yamlConsistencyJobEnv
ideally it would be consolidated in one place but .. this would be feedback for v2
p
yes, we need to preserve backward compatibility, in v2 we can think of some 
yamlConsistencyJobProps
that would group these
n
you could add the new fields already and use the new fields, with fallback to the old ones
p
hmm, I'd rather stay consistent with what we have now, it's consistently ugly 🙂 in v2 we'll make it consistently nice
n
btw is there any reason the env needs to be a linkedMap ?
p
I wanted deterministic order of the items, so that between subsequent regenerations, the YAML stays the same. I think it turned out that with regular 
mapOf
, the order is also de facto deterministic, but I haven't checked if the API guarantees it. So for now it's a bit more verbose 
linkedMapOf
, but for v2 I'm planning to check if we can make it regular maps across the API
n
nice
p
n
next i would look into ways to provide extensions to make modifying the workflow and setting up the environment easier so you do 
val workflow = workflow() {}.reportVersionUpdates()
or such
p
☝️ your PR adds 
fun Workflow.reportAvailableUpdates(...)
, isn't it the same?
n
but it does currently not modify the workflow to add the env
and i think of providing 2 variants.. one that makes it run in the consistency check job.. another for adding a separate job
p
Ah, I get it now - you'd like the extension function to add the token to the consistency check job, too? How about we skip it for now and make the user configure it manually? Just for simplicity, to avoid scope creep, then we can gather feedback and improve iteratively
n
sure, we cna skip that and just use it the way i have ben testing it so far
👍 1
btw, i tried to look at the javadoc jar since i added some kdoc bits.. but it seems like that jar is empty and the dokka tasks fail due to some task dependency things, might be worth looking into or fixing ?
how would you pass additional steps to the consistencyCheck job ? the steps we need to run needs to be after checkout and before the script gets executed
p
Re: Dokka, it seems it works fine during docs deployment, see https://typesafegithub.github.io/github-workflows-kt/api-docs/
Re: Extra steps to the consistency check job: we need another argument and I'd make it a function with receiver of type JobBuilder where one could add any required steps. LMK if you also want to contribute this one
n
will also contribute that one.. should i add it to the PR that adds the env ?
👍 1
p
Separate one please :) think of it this way: every PR lands as a separate release notes entry. It would be nice to explicitly announce this new extension point
n
okay and.. the PR should be there too
👍 1
and its being tested via 
snapshot-test.main.kts
depending on the snapshot, setting up java and running 
publishToMavenLocal
p
left a small comment in the one related to 
...env
n
merged the new test case into the existing test
❤️ 1
p
@Nikky could you split https://github.com/typesafegithub/github-workflows-kt/pull/1414 into 2 PRs: the API change, and adding the new workflow? or you can even just leave the API change in this PR, and I'll add the tests myself in the evening - I have some comments there, so I'll adjust. I don't want to make you tired of all these PR splits 😄 so excited about the true integration tests! 😄
so basically please remove changes in 
.github/workflows
, apart from this the PR looks good!
n
sure
p
BTW the test workflow failed: https://github.com/typesafegithub/github-workflows-kt/actions/runs/8816642835/job/24204510902?pr=1414 - if you have an idea why, let me know, otherwise I'll check later
n
cause i was stupid and copy-poasted the file.. it has the dependency on 
io.github.typesafegithub:action-versions-updates
which.. yeah.. not on this branch
was present locally ofc, so i did not notice the mistake
p
ah got it, sure!
conflicts in your PR 😅
n
merged main into the PR and resolved the conflicts
thank you color 1
✅ 1
p
@Nikky I've added the test workflow that uses the libraries pushed to maven local, would you like to add your new module to https://github.com/typesafegithub/github-workflows-kt/blob/c8962eb041b5cbc769d87a5211569d74e9db587c/.github/workflows/end-to-end-tests.main.kts#L44? useful to test all these issues with BOM/GMM
n
would need to be in a different file, to have outdated libraries in it .. or maybe it can be faked, but then the line number lookup will fail
p
ah ok, let's just leave it as is for now
LMK when the PR is ready for review, I guess the only thing to clarify is if we're using grouping the messages in the log, see my resposne
to have outdated libraries in it
wait, there's still some value in running this new module on a workflow with up-to-date actions! it just tests the logic and ensures it doesn't fail
n
that's true, let's do that then
🙌 1
now would be the time to talk about ... artifact name.. 
action-versions-updates
was just what i came up with so i could start prototyping maybe we want to change it ?
👍 1
p
I think it's good to start with a description of what this module's responsibility is, what it does. Then, maybe some better name with emerge
action-updates-checker - describes what it does (checks updates) for what (actions)
@Vampire do you like naming things? 🙂 we're looking for a name for a module that will basically fill in for refreshVersions, just for action bindings. The API is that the user calls a single Kotlin function in the workflow, and the output goes to the logs
n
i like 
action-updates-checker
, its better than what it had before... also.. should the package for the single (maybe 2) exposed fnctions still be 
[...].workflows.updates
?
p
a single function can find multiple updates. Or we can go with 
updating
, no strong opinion here
n
the times that mattersi s in the imports.. so 
import io.github.typesafegithub.workflows.updates.reportAvailableUpdates
or 
import io.github.typesafegithub.workflows.updates.reportAvailableUpdatesInConsistencyCheck
v
do you like naming things?
Not particularly, that is one of the hardest things in software development. 😄 Just look at how creatively I called my command framework. 😄
that will basically fill in for refreshVersions, just for action bindings
Would probably more handy to just extend refreshVersions with the functionality if possible. 😄
action-updates-checker
does not sound too bad to me 🙂
👍 1
p
Would probably more handy to just extend refreshVersions with the functionality if possible. 😄
I think Nikky tried that, but abandoned the idea?
n
also.. considering adding these functions as utilities. .so that adding this is just chaining it on the workflow builder https://github.com/NikkyAI/github-workflows-kt/blob/dev/action-updates-checker/src[…]/kotlin/io/github/typesafegithub/workflows/updates/Reporting.kt
no, you requested it to not be bundled in with refreshVersions.. i guess partially because this adds dependencies on ktor..
v
He didn't want it bundled into github-workflows-kt
p
I'd rather not bundle this functionality in github-workflows-kt
github-workflows-kt, not refreshVersions 😄 I don't care what happens in refreshVersions
n
eh right.. i misread the message.. brain is still waking up it seems
p
but we won't discard the PR worth several days of our work, right? 😛
I'm happy to keep the module you created, let's see how it works in practice
if one day refreshVersions supports this use case, we can deprecate it
n
lets keep it.. if it turns out something useful and can be invoked from refreshVersions in the future.. lets add that or migrate it.. the code will largely translate
👌 1
p
I'm close to approving the PR, is there anything left to discuss?
n
the utilities for easier setup i linked earlier?
how do you feel about including those ?
p
what do you mean by utilities? could you show an example?
n
p
ok, so I'm planning to go forward with Make workflow definition declarative soon, so writing to YAML will happen inside 
workflow { }
, and there will be some extension points so that one can tweak the 
Workflow
object before dumping to YAML
so for now I'd stay with the current API, and wait for 2.0 to see how it evolves
n
ah okay.. so we would rewrite this anyways, got it
then i don't think anything else is left other than... guides and docs ?
p
CI failed in the PR 😄
other than that, I'd merge the PR as is and follow up with a separate page in the docs under "User guide" + marking that the API of this particular module is yet unstable
n
missing a apiDump after i moves stuff around, classic
👌 1
the PR should not fail (at this step) anymore..
sounds good to me
p
@Nikky I cannot find messages from the new module, and see this https://github.com/typesafegithub/github-workflows-kt/actions/runs/8831625234?pr=1393 
github token is required, but not set, skipping api calls
Even if there are no actions to be updated, shouldn't there be some message from the new module showing that it actually ran?
n
i guess i should maybe add a message and write into the summary, yep
🙌 1
ah its the kotlin logic step later...
👍 1
i need to not execute the reporting when its kotlin logic step.. and also report when stuff is up to date rn. it just runs whenever the script gets invoked
👍 1
p
Easy to check by presence of 
GHWKT_RUN_STEP
env var
n
added a check in the end-to-end test and made it print into log and summary when stuff is all up-to-date
p
ok, one nit comment
I think this check will always go together with calling the new function, so it can be done at the beginning of it
n
i think so too.. but not 100% sure if people might want to run it in a kotlin logic step
p
inside? nah, it should be run on the 
Workflow
object, at the time of generating the workflow, not executing it
n
good :3, and done
👍 1
is there ANY way for the new dependency to not also provide 
worklfows-kt
to the script? this kind of bothers me a bit.. that now people wouldnot need to add the main dependency because the new module adds it transitively..
p
mark it as 
compileOnly
, I think
instead of 
implementation
n
will try, it worked
testing scripts against 
mavenLocal
makes this so much faster
🦜 1
p
yeah, thanks to you ☝️ ! regarding 
compileOnly
for also 
shared-internal
, I'm afraid it may not work because if we ever drop depending on it from the main module, it won't be pulled in by anything
n
well .. one comment i noticed that should have been removed.. probably another weird merge / rebase behaviour
p
I think we should actually stay with 
implementation
, and make a fair assumption that the new module is used as a supplement for the main one
n
it does work because workflows-kt provides it ? well it works locally
p
yes
let's just stay with 
implementation
👍
n
okay, will revert the change
👍 1
something to note is that right now this will mess up workflows that print the yaml since it also prints to stdout that might be worth mentioning in the docs fixing this is not worth it imho, the consistency check would not be able to do logging and only have the summary to write to on CI
p
you mean the old API where the script printed the YAML on stdout?
n
yep
p
I think we shouldn't bother at all, this behavior was let go long time ago
the API now is that the Kotlin script should produce the YAML file under a well-defined path
n
technically it is still supported in the consistency check .. thats why i was reminded
ahh.. the times before 
__FILE__
p
sure, let's just document it, I need to sort it out in 2.0
n
i fixed the linting errors that my change introduced (idea plugin disabled locally since it iust started reformatting the 
.main.kts
scripts whenever it wanted)
or so i thought.. luckily there is a gradle task for it ... (i only ran check before to see if it passes or not)
p
yes, ktlint is configured
shared one last nitpick comment and we're merging this! 🚀
n
removed the serialization plugin, was a leftover from the first draft
👍 1
p
merged 🦜 kodee greetings
n
yay
p
I'll release the libs next week, now you can play with it through the snapshots
n
nice, thanks
I think I can wait about a week and schedule updating all our workflows for myself
👍 1
p
thanks for your effort!
6 Views
Open in Slack
PreviousNext
Powered by