https://kotlinlang.org logo
Join Slack
Powered by
Hello folks, I suggested yesterday a refactoring o...
# github-workflows-kt
e
Hello folks, I suggested yesterday a refactoring of the main APIs, summarizable in the following: From:
Copy code
val workflow = workflow( 
    name = "test"
    on = listOf(Push())
) {
  job(
    id = "test_job"
  ) {
     // ...
    } 
}

workflow.writeToFile()
To:
Copy code
workflow {
 name = "test"
 on = listOf(Push())

 job {
   id = "test_job"
     // ...
   }
 
 writeToFile()
}
My motivation for this is that it is more in-line with gradle’s dsl and closer to the original yaml syntax. You can read more details here: https://github.com/typesafegithub/github-workflows-kt/issues/874
plus1 1
p
hi Ennio, happy to see you on Slack! there’s a small thing to fix in your message: in the “From” example, you’re missing either 
print(workflow.toYaml())
, but that’s actually discouraged - nowadays we do `workflow { }.writeToFile()`(fixed the docs here). It’s important to get the full picture of how the YAML gets into the actual YAML file
we’re looking for some feedback on the newly proposed approach as both have their pros and cons. For me, the difference is subtle, and I stated my way of thinking in the ticket
e
Thanks @Piotr Krzemiński updated, looks better now?
p
yes, it’s correct now, but I usually write
Copy code
workflow( 
    name = "test"
    on = listOf(Push())
) {
  job(
    id = "test_job"
  ) {
     // ...
    } 
}.writeToFile()
which makes it a bit shorter. Probably a matter of style now
e
ah yes, it can of course be called from inside or outside, I guess it really depends on developers preference 🙂
p
so now the argument boils down to hiding 
.writeToFile()
inside the 
workflow
function, right?
oh, I see now - you actually propose to call 
writeToFile()
inside 
workflow { }
sorry, I haven’t noticed it before
to be honest, I don’t like it that much 🙈 `workflow { }`is focused on building an immutable object, and we’d add a call with side effects inside. It would violate some of the assumptions/rules I try to stick to
but I’m happy to learn others’ take on this
e
I don’t really get your point: the Workflow object would still be immutable, the side effects are at the OS. If you consider these as side effects, it would be the same when called outside of the 
workflow {..}
, wouldn’t it?
p
yes, what I mean is that such construct:
Copy code
workflow {
    // ...
    writeToFile()
}
looks like 
writeToFile()
is a part of the process of building the workflow object, and it’s not - it’s storing the object in a serialized form, so in my head it doesn’t fit the curly braces for 
workflow { ... }
- do you know what I mean?
it’s like: • 
workflow { }
- build the workflow • 
.writeToFile()
- store the file seems logical
initially I thought you were proposing to hide 
writeToFile()
inside the implementation of 
workflow { }
which would kinda simplify the workflow file, and it actually made more sense to me because people usually do it
e
ah sure, that was my original proposal, but I understood you suggested to keep it explicit
I still believe that these modifiers should be handled completely outside of the script file tbh
p
what do you mean by modifiers?
e
toYaml
, 
writeToFile
, etc.
p
yes, it makes sense, at least in the default form + leave API for fine-grained control over the YAML
@Vampire @LeoColman @jmfayard what’s your take on this? ☝️ do you use 
toYaml()
in your workflows in some fancy way, or it’s just building the workflow and storing/printing it?
@Ennio Visconti would you be able to prepare a PoC PR? it would let us foresee some problems
the variant with 
writeToFile()
hidden in the implementation of 
workflow
makes most sense to me
e
sure!
v
I don't use 
toYaml
afair as it produces a different result, e.g. regarding the consistency check. When I had to modify the result, e.g. before the explicit preamble support, I used 
writeToFile
, then read the file modify and save again.
But remember, that the function also takes arguments
p
Yeah, they would have to be moved to 
workflow
call, preserving current defaults. I'm also tempted to remove 
toYaml()
altogether
v
Wasn't 
toYaml()
for example for cases where someone has some other management / system from which he uses the lib instead of using it from a 
.main.kts
file? I vaguely remember at least one conversation here about such a case.
I think this conversation of @Damien O'Hara is what I had in mind: https://kotlinlang.slack.com/archives/C02UUATR7RC/p1675057138707659
p
Right, let's better leave the API that doesnt depend on Kotlin Script and file IO
n
downsides of making assignments inside the lambda like
Copy code
name = "test"
on = listOf(Push())
is that you are not as sure that you passed in all the required parameters.. discoverability is more of a pain.. this would imho remove some of the benefits of writing this in kotlin if we can maintain both .. or it can be pubished as a separate optional library.. that would be less of a issue it does seem to look nicer… BUT i hate having to run code just to have the code crash at runtime, telling me that i missed some required parameter… probably without even giving me a line to look at.. or deeply hidden in some stacktrace that i have to pull out if the terminal with fucked up linebreaks
p
I’ve just noticed this ☝️ the choice of passing these values as function arguments was deliberate. As Nikky mentions, it lets us check in compile time if something is required
n
i am trying to push more of my coworkers to use this library.. and the main complaint is discoverability.. not mismatching with the github yaml.. after all who likes to look at at…
if there existed a compiler plugin that can be used with scripting that allows you to declare required assignments in lambdas… maybe this woudl be a option.. but.. 👀 nothing available as far as i am aware on either end
e
what do you mean by discoverability?
n
having the IDE list the parameters that you have to pass in… instead of typing 
this.
to hope for it to give you a list of all the properties and function that you could be calling on the current scope (which will be way more than the required params)
and might include extension properties and function too…
e
well, 
Ctrl
+ 
Shift
+ 
Space
does show you suggestions including extension functions in IntelliJ AFAIK
n
the problem is that you do not want all of the suggestions… mainly you care about getting it to work.. so you are about the minimum required properties.. and THEN you care about additional properties
e
I see
But do they get it in Yaml?
I think a key point is that you can at least get immediate error when you are typing a command that it doesn’t know, I don’t think you can get that easily in yaml
n
yes, why would you pass anything into a workflow or job if it did not influence the generated yaml in some way ?
i don’t think the point here is to argue between yaml and kt ?
the point i was trying to makei s that there is a good reason to have the required and most important arguments as named parameters and the rest in the lambda scope
e
aaaah, sorry! I get your point now, thanks!
I think it makes a lot of sense 🙂
n
i would not be against trying this out as a seperate library that provides wrapper functions though.. if you can figure out how to build that.. probably loads of delegation…
there might after all be benefits we are not seeing without trying it out 😛
p
I’m not planning to change this aspect of API before going stable (v1.0) later this year, but some new Kotlin mechanism appears to validate such usage in required cases, I’d be keen to try it
unfortunately, Kotlin Scripting is not yet equally well developed as Kotlin/JVM, so e.g. we don’t have straightforward support for compiler plugins
n
tbh thats probably the main painpoint holding me back from integrating more kts scripts into projects… even something as simple as serialization is just … a pain to work with on kts files.. to the point where its just not quite worth the effort… and writing the same thing in a standalone project if it needs to be kotlin or in a scripting language if it does not … turns out faster and less (or differently) painful every single time /rant
e
I don’t have such a negative feeling of Kotlin scripting… Probably because I remember how painful Gradle was in groovy 😂
n
kts for gradle is good… kts for standalone files is where the painpoints are
after all everyone uses .gradle.kts but very few people do complex tasks in .main.kts files
shudders in groovy .. the flashbacks…
d
I feel the same way about .kts. I was experimenting a while ago with integrating github-workflows-kt into a declarative file gen API I was building for generated documentation
n
it got so much potential and still the kts usecase looks like they left it alone after MVP stage
v
Scripting gets some love soon they said recently, so maybe it improves in the "near" future 🙂
d
n
thats really cool
d
the concept is workable I think
p
revisiting the original request where this thread came from: https://github.com/typesafegithub/github-workflows-kt/issues/874 and trying to summarise the outcome of our discussion. Feel free to share anything relevant there, if you agree/disagree with anything proposed there
Related: https://github.com/typesafegithub/github-workflows-kt/issues/1208 - I figured that for one feature I'm working on, the current API limits me. I'm considering switching to something more Gradle-like, declarative
2 Views
Open in Slack
PreviousNext
Powered by