https://kotlinlang.org logo
#github-workflows-kt
Title
# github-workflows-kt
v

Vampire

01/24/2023, 5:17 PM
Hm, I need
actions/cache/restore@v3
and
actions/cache/save@v3
. But I fear the generator will not work with that? 😭
p

Piotr Krzemiński

01/24/2023, 5:58 PM
that's correct, it's currently not supported. But it should be pretty simple, I've created a feature request: https://github.com/krzema12/github-workflows-kt/issues/646
@Vampire what name of the wrapper class would you expect such nested actions to have? (I have my idea, but I'm wondering what you propose without suggestions)
v

Vampire

01/24/2023, 7:36 PM
joined with camel-case
CacheRestoreV3
CacheSaveV3
p

Piotr Krzemiński

01/24/2023, 7:38 PM
Ok, and what would you say for
Cache_RestoreV3
? My intent is to show that this is a nested action, but now I'm not sure if it's worth showing. Thoughts?
v

Vampire

01/24/2023, 7:38 PM
So I guess you are currently enhancing the wrapper generator?
p

Piotr Krzemiński

01/24/2023, 7:38 PM
I'm planning to do so before the next release, didn't start yet
v

Vampire

01/24/2023, 7:39 PM
Hm, also an idea. Underscores just feel awkward in class names. 😄
Not sure, would probably also be fine
p

Piotr Krzemiński

01/24/2023, 7:39 PM
Yep, that's why it's a bit controversial :)
Ok, it's easy to change later, while still in unstable mode
Another idea:
CacheV3.Restore
-
Restore
being a nested class of
CacheV3
Expresses nesting in a clean way IMO
v

Vampire

01/24/2023, 8:07 PM
Hm, also a good idea. But you have to make sure that
CacheV3
can be a completely own action in itself, or that it can only be a holder.
p

Piotr Krzemiński

01/24/2023, 8:08 PM
Yes, true - adds to the complexity of the code generator a bit
v

Vampire

01/24/2023, 8:08 PM
What if the action is in
my/littl/action/action.yaml
will it then be
My.Little.ActionV0
?
Not objecting, just noting
p

Piotr Krzemiński

01/24/2023, 8:09 PM
Following my example, it would be
MyV0.Little.Action
. Looks a bit weird, doesn't it? This version in the first part. But it matches the fact that the root thing is versioned
v

Vampire

01/24/2023, 8:10 PM
Ah, right
Yeah, and if there is actually something in the upper level too, it is also arguable that the version is there.
p

Piotr Krzemiński

01/24/2023, 8:12 PM
Sorry, didn't get it. Could you rephrase?
v

Vampire

01/24/2023, 8:14 PM
Yeah, weird sentence, sorry 😄
If My is an action itself and Little and Action are sub-things that are versioned together with the tag like for the cache action, it also seems ok to have the version there the front
Definitely needs some getting used to it, but maybe not too bad and probably definitely better than underscores 😄
p

Piotr Krzemiński

01/24/2023, 8:16 PM
Cool, so I think I'll go this path :) thanks for the brainstorming
v

Vampire

01/24/2023, 9:57 PM
Btw. is the type for outputs relevant In the context of your lib? I've e. g. seen in the cache types that you have string as output and a comment to check from the description and set accordingly. But even if I set it to
boolean
which it should really be, the generated class does not change of course, the accessor only gives the expression string for it anyway.
p

Piotr Krzemiński

01/25/2023, 6:04 AM
It's the way it currently works, the outputs are basically untyped (string) GitHub expressions. The only benefit of having them generated is being able to rely on the output names, which gives some level of safety (probably not yet type-safety)
It's related to https://kotlinlang.slack.com/archives/C02UUATR7RC/p1674583564743569?thread_ts=1674583564.743569&cid=C02UUATR7RC in a sense that one day I'd love to have proper type-safety when passing values between outputs, inputs and other parts of the API
v

Vampire

01/25/2023, 8:34 AM
Yeah, as I said, I think you need denotable union types. Then you can declared the fields as taking a
boolean
or a
BooleanExpression
and the output would supply the latter which could also be used for the Gradle cache action example. In the meantime you could probably just have two properties or two setters each taking one of them. Or alternatively you could use
Either
from Arrow or make an own version. But that would make the direct setting less nice.
p

Piotr Krzemiński

01/25/2023, 8:39 AM
my plan is to wait for some development of events in [KT-13108] Denotable union and intersection types . If it doesn’t happen (soon-ish), I considered both approaches you mention (several arguments per input, depending on the use case) or something similar to
Either
, but with more specific “sides” like
typed
and
untyped
v

Vampire

01/25/2023, 8:41 AM
Yeah, thats the issue I talked about. But I somehow don't expect it to happen soon-ish, just anytime-ish. 😄
I guess in the light of the denotable union types for mid-term it would be better to have two separate properties. Then when denotable union types finally arrive the typed field value settings can stay the same and just the others need to be changed, probably by simply removing
Expression
from
myFieldExpression
. If you have something like a specific
Either
you will break all usages when switching to denotable union types.
I've seen your new branch. Did you change mind and not make the "subactions" inner classes? Or is it just an intermediate step that just worked the quickest and you will change it? 🙂
p

Piotr Krzemiński

01/25/2023, 8:58 AM
I’ve slept on the the idea of having
FooV1.Bar
and figured it would be pretty complex to implement in a proper, generic way + one day I’m hoping to create a complier plugin that would generate the wrappers, and I’m not sure how easy it would be to generate a single class with nested classes for more than 1 requested action wrappers. That’s why I decided to go with something simple that I almost have implemented. I prefer to put the effort in more crucial features
once it’s rolled out, I’m still open to feedback - maybe the simplified implementation creates some issues. For now the only problematic cases I can think of are e.g. clashes between
some-action/foo/bar-baz
and
some-action/foo-bar/baz
which will lead to generating the same class name, but I assess the risk of getting these as low
(I should’ve updated the ticket regarding changing my mind, sorry)
another argument is being in line with what you intuitively came up 🙂 https://kotlinlang.slack.com/archives/C02UUATR7RC/p1674589041434689?thread_ts=1674580678.412759&cid=C02UUATR7RC this intuition is important to me
v

Vampire

01/25/2023, 9:05 AM
Yeah, all is well, was just curious. Definitely no excuse needed, especially when you immediately start working on things I want to have. :-D
p

Piotr Krzemiński

01/25/2023, 9:06 AM
sure, but it’s good you ask - I like having design decisions documented 🙂 and I like being challenged (just not too much, to not lead to DoS)
v

Vampire

01/25/2023, 10:37 AM
I guess in the light of the denotable union types for mid-term it would be better to have two separate properties.
Maybe something like this will work nicely: https://github.com/Vampire/github-workflows-kt/commit/4ed978461cfbfeb4d9ae0ff2175f1f45de0f17b9 Not sure about the `List`s though
p

Piotr Krzemiński

01/25/2023, 1:30 PM
thanks, I’ll have a look later!
and back to the original topic, these two are merged and will be released next week: • add actions/cache/save@v3add actions/cache/restore@v3
v

Vampire

01/26/2023, 2:26 AM
and back to the side topic, here a bit more complete version with working generation, but completely untested: https://github.com/Vampire/github-workflows-kt/commit/87efab0e297cfceea5deebce256f75887101e331 At least it compiles and (with some modifications) the tests pass 🙂
Copy code
val foo = uses(action = SetupWslV1())
val bar = SetupWslV1(
    distributionExpression = foo.outputs["bar"],
    useCacheExpression = Expression("1 == 2"),
    updateExpression = foo.outputs.wslShellDistributionWrapperPath as Expression<Any>,
    wslShellCommandExpression = foo.outputs.wslShellDistributionWrapperPath
)
p

Piotr Krzemiński

01/26/2023, 7:25 AM
looks interesting 👀 I have it in my queue to look at it, but now I need to take several days off form the project, I'm out of budget because of this coding frenzy 😉
v

Vampire

02/03/2023, 8:43 AM
As you are back, did you have a look? 🙂 Btw. I'd prefer you adding tests for all that. I already put too much time into it. 😄 And it would allow to test the changes with a fresh look on it. 🙂
p

Piotr Krzemiński

02/03/2023, 8:44 AM
I’m not yet gone actually 😛 recent contributions kept be busy, I was planning to take a week off form working on the lib now when the release is out
test: some kind of tests are in place, let me know which ones you mean exactly
v

Vampire

02/03/2023, 8:50 AM
Oh, haha, sorry. 🙈 That change adds significant changes and functionality, I guess there should be some tests testing whether the type-safe wiring works like expected and so on. :-)
p

Piotr Krzemiński

02/03/2023, 8:51 AM
this feature competes with strategy matrix support - what would you say is more impactful to you?
fortunately the typed inputs is a simpler, incremental feature, so we can try to get it through sooner
v

Vampire

02/03/2023, 9:05 AM
typed outputs you mean probably. 😄 Now that you mention it, I guess the typed outputs does not compete with the matrix support, but is a prerequisite. What you get from the matrix is an expression, so if you want to use matrix values for non-string fields, you need at least something similar. It probably needs to be expanded even, so that you can use outputs or matrix values for other properties too. It currently only works for
String
fields.
p

Piotr Krzemiński

02/03/2023, 9:06 AM
ideally we’d have a comprehensive design effort for this kind of type-related stuff, so that all is consistent with each other 🤔 but yeah, time 😅
v

Vampire

02/03/2023, 9:07 AM
Which to tackle first best I don't know, but I also guess the typed outputs will be simpler and faster through
p

Piotr Krzemiński

02/03/2023, 9:07 AM
yes, a good way to start
38 Views