https://kotlinlang.org logo
Join Slack
Powered by
..., how would I write this in a working way? ```l...
# github-workflows-kt
v
..., how would I write this in a working way?
Copy code
lateinit var deploymentStep: ActionStep<DeployPages.Outputs>
job(
    id = "deploy",
    name = "Deploy Site",
    runsOn = UbuntuLatest,
    needs = listOf(buildSite),
    permissions = mapOf(
        Pages to Write,  // to deploy to Pages
        IdToken to Write // to verify the deployment originates from an appropriate source
    ),
    environment = Environment(
        name = "github-pages",
        url = expr { deploymentStep.outputs.pageUrl }
    )
) {
    deploymentStep = uses(
        name = "Deploy to GitHub Pages",
        action = DeployPages()
    )
}
This and hoping that the step id naming schema doesn't change cannot be the solution, can it?
Copy code
job(
    id = "deploy",
    name = "Deploy Site",
    runsOn = UbuntuLatest,
    needs = listOf(buildSite),
    permissions = mapOf(
        Pages to Write,  // to deploy to Pages
        IdToken to Write // to verify the deployment originates from an appropriate source
    ),
    environment = Environment(
        name = "github-pages",
        url = expr("steps.step-0.outputs.page_url")
    )
) {
    uses(
        name = "Deploy to GitHub Pages",
        action = DeployPages()
    )
}
p
> This and hoping that the step id naming schema doesn't change cannot be the solution, can it? I'm not planning to change these, but I agree that it would feel wrong as we would expose something internal. I was curious if someone solved this idea, and they did (1, 2), with 
_customArguments
. I don't remember why the step ID isn't customizable, maybe for simplicity: to not have to check for uniqueness? Anyway, I'm for allowing customizing step ID (here) to handle such cases. A PR is greatly welcome, and if you cannot post one, let me know - I'll do it within several days and release the lib ☝️ this may not be the most elegant solution, but I don't have a better idea
v
Ah, forgot that I can overwrite using custom arguments, that's at least better then relying on naming convention. I'm also pro-id-customizable. But it would still be sub-optimal, as you cannot use the 
ActionStep
instance and thus the 
Outputs
object. 😞
p
yeah, the problem here is that this use case breaks a certain assumption: steps cannot be accessed in workflow configuration. It's an unusual thing of GitHub Actions that properly supporting might turn the DSL upside down. I'd call it an edge case, and just this one isn't a compelling reason to devote a significant portion of time to design something better, but if you or anyone has an idea, feel free to shre
v
Ah, slightly nicer:
Copy code
val deploymentStepId = "deployment"
job(
    id = "deploy",
    name = "Deploy Site",
    runsOn = UbuntuLatest,
    needs = listOf(buildSite),
    permissions = mapOf(
        Pages to Write,  // to deploy to Pages
        IdToken to Write // to verify the deployment originates from an appropriate source
    ),
    environment = Environment(
        name = "github-pages",
        url = expr(DeployPages.Outputs(deploymentStepId).pageUrl)
    )
) {
    uses(
        name = "Deploy to GitHub Pages",
        action = DeployPages(),
        _customArguments = mapOf(
            "id" to deploymentStepId
        )
    )
}
p
💯 for type-safety in 
expr(DeployPages.Outputs(deploymentStepId).pageUrl)
😄
v
And for a "proper support" maybe only two things are necessary. An 
ActionStep
constructor that does not need the ID twice and 
uses
accepting an 
ActionStep
.
Copy code
val deploymentStep = ActionStep(
    id = "deployment",
    name = "Deploy to GitHub Pages",
    action = DeployPages()
)
job(
    id = "deploy",
    name = "Deploy Site",
    runsOn = UbuntuLatest,
    needs = listOf(buildSite),
    permissions = mapOf(
        Pages to Write,  // to deploy to Pages
        IdToken to Write // to verify the deployment originates from an appropriate source
    ),
    environment = Environment(
        name = "github-pages",
        url = expr(deploymentStep.outputs.pageUrl)
    )
) {
    uses(deploymentStep)
}
p
I actually like it! pretty elegant - best we can do, I think
so we have 2 PRs to make 🙂 let me know if you're willing to contribute, or I should handle this
v
I'll see whether I find time. Ah, we even do not need the outputs provider, the action already provides the provider method.
👍 1
p
v
Great, was about to create issues myself right now 🙂
👍 1
p
@Vampire all changes you reported are in the latest snapshot: 
3.1.1-20250122.074629-13
- feel free to give it a spin and let me know if you need anything else before I release
maybe I'll also go with [Core feature request] Support types for all triggers, but need to think to what extent it would be a breaking change
e.g. for the popular Push, we'd need to make 
types
the last argument because named arguments aren't enforced here
v
Very nice, thanks. Then I have to admit I'm not going to contribute those. 😄 All I'm aware I need for the workflows I'm currently working on then is https://github.com/typesafegithub/github-workflows-kt/issues/614, or at least the 
inputs
part.
need to think to what extent it would be a breaking change
Well, same with any data class I guess which is why one shouldn't use data classes in API.
But as most typically the lib is used in scripts, it probably is not that bad, unless someone writes some utility lib on top that is pre-compiled
Technically even the 
Release
change was breaking already
👌 1
p
All I'm aware I need for the workflows I'm currently working on then is https://github.com/typesafegithub/github-workflows-kt/issues/614, or at least the 
inputs
part.
open to contributions here, unless you're ok with waiting several days
v
I'm not too much in a rush, just preparing an evangelizer PR to 
kotlin-wrappers
. 🙂 Would be nice if a Kotlin-related JetBrains project uses the lib, wouldn't it? 🙂
p
sure it would! it would give good exposure
v
Well and for that I want to avoid 
_customArguments
of course 🙂
👍 1
p
the 
WorkflowCall
thing needs a bit of design, so let me take a few days to think
v
Tell me, what design is necessary? How is it different to 
WorkflowDispatch
?
p
I just meant to avoid a breaking change, but on the other hand, it would be a similar kind of a breaking change like with 
Release
. Regarding https://github.com/typesafegithub/github-workflows-kt/issues/614#issuecomment-2606604916, for now I'd just create a dedicated 
Input
class for 
WorkflowCall
in some context having a common 
Input
class would be nice (like, defining inputs just once and passing them to 
WorkflowCall
and 
WorkflowDispatch
- it's a valid use case, right?), but I'd consider it in 4.x
👌 1
v
Then the change is probably trivial for now :-)
p
yes! the design phase is done 😄
v
But yes, for that workflow I'd use a common input as it has both with the same input
p
oh, so this use case already materialized. Maybe then it wouldn't be that bad to use 
WorkflowDispatch.Input
in 
WorkflowCall
? Then we could clean it up in 4.x by moving the 
Input
class outside. I'm thinking if both inputs can diverge at some point - technically they can
I see there's 
on.workflow_dispatch.inputs.<input_id>.required
, but there's no 
on.workflow_call.inputs.<input_id>.required
- so they're not equal already
v
Oh, ok, then they seem to have a superfluous field and it should indeed be separate
👍 1
p
ok, I'll implement it within several days (open to contributions 😉 )
❤️ 1
eh, here they show that 
required
is valid for an input in 
workflow_call
. Messy 😄
v
Well, or the example is wrong. 🤷‍♂️ I guess we should have 
required
but even in 4.0 keep the input classes separate, as it is not you that controls that they stay the same but GH could any time change that.
👍 1
p
@Vampire please review: feat(library): add properties to WorkflowCall trigger (last one before releasing?)
v
Ah, another reason for separation, there is a difference. The types are different. workflow dispatch additionally supports environment and choice and for the choice options
👌 1
Changes look good to me from a cursory look. Two points though. Not directly related to the PR, but workflow dispatch type is missing "number". And shouldn't the type maybe be within 
Input
(in both cases), or called 
InputType
?
p
> workflow dispatch type is missing "number". good catch! fixing here > And shouldn't the type maybe be within 
Input
(in both cases), or called 
InputType
? yes, now it's like 
WorkflowCall.Type
, as if there were types of workflow calls. To be renamed in 4.x (issue)
👌 1
I'm tempted to assign a "level/likelihood of breakability" for a given change in the release notes 😄 Some changes are technically breaking, but highly unlikely to actually break some consumer. Some, on the other hand, rename a popular symbol or change how YAML is formatted - then 100% breaking
all right, all changes for the next release candidate are merged - @Vampire good to release or you need anything else? 🙂
how about creating the kotlin-wrappers workflows with the snapshot version, and once you've got all of them rewritten with no missing features, tell me to release?
v
Did you push a snapshot to Sonatype, then I'll test with that right now
p
it gets auto-published via 
Build / Publish snapshot (push)
. Since it doesn't log the version, I always go to https://s01.oss.sonatype.org/content/repositories/snapshots/io/github/typesafegithub/github-workflows-kt/ and check myself 😄 this time it's 
3.1.1-20250122.115243-16
(I'm going to release as 
3.2.0
)
v
Makes sense, as features were added
You only need the base version unless you really want a specific snapshot, so this should usually work:
Copy code
@file:Repository("<https://s01.oss.sonatype.org/content/repositories/snapshots>")
@file:DependsOn("io.github.typesafegithub:github-workflows-kt:3.1.1-SNAPSHOT")
I'll try
👍 1
Hm, interesting. 
3.1.1-SNAPSHOT
resolves, but does not have the new code o_O
p
maybe you have something cached?
v
I don't think I resolved a 3.1.1-SNAPSHOT before or published to maven local
Hm, no, even with 
3.1.1-20250122.115243-16
explicitly it does not work o_O
p
Copy code
unzip github-workflows-kt-3.1.1-20250122.115243-16.jar | grep "WorkflowCall$Input"
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Secret$$serializer.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Output.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Type$Companion.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Input$$serializer.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Companion.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$$serializer.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Type.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Secret.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Output$Companion.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Input$Companion.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Output$$serializer.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Input.class  
  inflating: io/github/typesafegithub/workflows/domain/triggers/WorkflowCall$Secret$Companion.class
the new classes are there
v
Argh, I fell into the trap again. No conflict resolution between importing and imported file. I only had the lib in the common file. In the importing file it was 3.0.0 from the dependencies of used actions.
👍 1
Grml, no, still does not work, something I do wrong
Ah, the other trap, the lib has to come before the actions or still the transitive wins
🙈
p
yeah, the actions specify a dependency on the latest released version: https://github.com/typesafegithub/github-workflows-kt/blob/3e6fd5aa29012a399f834ed[…]/io/github/typesafegithub/workflows/mavenbinding/PomBuilding.kt (I'm still not sure if it's the best way to go, but "it works" somehow now)
v
That's fine, it is just the main.kts non-sense that works badly. I think I opened issues for those too.
👍 1
p
so far not much happening in the issues after the last year's article 😢
v
Hm, I wonder whether inputs outputs and secrets should be nullable and null by default
Copy code
$ git d .github/workflows/*.y*
diff --git a/.github/workflows/publish-github-pages.yaml b/.github/workflows/publish-github-pages.yaml
index e68927dd6..c6247bd5f 100644
--- a/.github/workflows/publish-github-pages.yaml
+++ b/.github/workflows/publish-github-pages.yaml
@@ -16,6 +16,8 @@ on:
         description: 'The branch, tag or SHA to checkout. See actions/checkout ''ref''.'
         type: 'string'
         required: false
+    outputs: {}
+    secrets: {}
   release:
     types:
     - 'published'
p
oh! it didn't come up in the tests, I intended to hide 
outputs
and 
secrets
if they're not set. Let me adjust
v
inputs is also optional, isn't it?
That reminds me of another potential improvement for the future. Some way to access the inputs later on. Right now it is 
expr("inputs.checkout-ref || ${github.ref}")
p
fix(library): do not emit WorkflowCall outputs and secrets if none set - I know that 
inputs
don't behave the same way, but I don't want to break folks that have zero-input workflows...
v
Hm, maybe that is it. Though in the original description it uses 
event.inputs.version
in the work-around while afaiu it should be 
inputs.version
, and he does not declare an input called 
version
but an input called 
RELEASE_VERSION
. And the suggested solution with "After the change, the inputs wouldn't be defined here" might be sub-par as inputs can come from 
WorkflowDispatch
or 
WorkflowCall
afaiu.
Unless you do two, 
Contexts.workflowDispatchInput
and 
Contexts.workflowCallInput
. Both might have different inputs.
p
to be discussed under the issue perhaps? this thread is already getting long 😅
🙈 1
👌 1
I need to drop for a while - I've merged the PR with removing 
outputs
and 
secrets
if they're not set, so feel free to try the snapshot once it appears in a few minutes, and let me know if you see any other rough edges!
v
Looks fine I'd say. Unless I overlooked something I just miss the types for Vitalik and the type-safe inputs handling. Result is as expected now too without inputs, outputs, and secrets.
thx for the quick action 🙂
And just for reference again, https://youtrack.jetbrains.com/issue/KT-69145/ is what I hit with the new code not being available. Reading my analysis I again just shake my head in exasperation 😄
👍 1
p
are you planning to contribute typings to https://github.com/karakum-team/vitalik? there are no inputs or outputs 🤔 so you'd just get rid of 
_Untyped
from class name, right?
v
Exactly
Declaring that there are no inputs and outputs is as fine as declaring them. If you think about detecting that situation and treating it like empty typings, that is not a good idea on 2nd thought, because as soon as an input is added later the detection would flip over and the typed class without in- / outputs suddenly vanish breakingly.
p
yeah, it crossed my mind, but then I recalled the argument you're mentioning - we accounted for this edge case when designing the 
_Untyped
wrappers
do you want to contribute the empty typings, as a part of your effort? 🙂
v
That's the plan, yes, unless you beat me to it there too 😄
p
I'll leave it to you 🙈 😄 I'm wrapping up my coding part of day
v
They also know me as contributor already, so maybe that increases chances of getting merged. 🤷‍♂️
💡 1
Aaaargh, the validator action does not like an empty typings file
Well, not empty, that is tested, but only having a comment triggers it
p
Thanks for fixing the issue in typing validation action - I've released a patch release
👌 1
To be checked if the bindings server is also affected by this issue
v
We'll see latest when the PR https://github.com/karakum-team/vitalik/pull/1 gets merged if it gets merged 🙂
Yep, exact same problem 😞
> Task action binding generatortest
io.github.typesafegithub.workflows.actionbindinggenerator.typing.TypesProvidingTest > copes with comment-only typing FAILED
com.charleskorn.kaml.EmptyYamlDocumentException at TypesProvidingTest.kt:35
p
it was easy to anticipate, unfortunately we have this duplication right now (between the server and github-actions-typing)
v
Well, they are totally different projects 🙂
p
Thanks! Will deploy outside of the schedule once the typings are merged. Normally it will be deployed on Sunday
v
Why not doing it right away? Not that someone gets the idea to use it before you deploy 🙂
p
I generally try to minimize the deployments to avoid downtime. But maybe you're right, in this case - let me deploy
👌 1
Thanks for today's fruitful cooperation 😊
v
Same :-)
2 Views
Open in Slack
PreviousNext
Powered by