:github-workflows-kt: <v2.3.0 released!> :warning:...
# github-workflows-kt
p
github workflows kt v2.3.0 released! ⚠️ This is the last release with bundled bindings. The next release is going to be 3.0.0, with the bundled bindings removed. Please switch to Maven-based bindings server, see Migrating to Maven-based bindings
🎉 1
v
The problem with the binding server is, that it is a step backwards from the org title. Because if you added an action explicitly to the lib, you either made the owner add typings or had to add the typings to the lib or schema-repo. Now you can just use and action and live with everything being String-y. And we both know that developers are lazy, don't we? 😄
How about some itching like making Actions that are generated without full and recent typing information (so also if there is some but outdated) deprecated? Or some different naming like prefixed with
UnTyped
or similar. The latter would of course be a breaking change and require a new binding server version route. 😄
Also, how do you test your typings? Before you could just get the lib, add the typings and generation and test with that. Is there some new way? Maybe even something magically convenient like making a special request to the binding server with the typing as POST body that then generates a specific UUID version of the lib that can be used to test the newly generated action and then maybe also some website form to do it in a browser.
p
The problem with the binding server is, that it is a step backwards from the org title.
IMO it depends which metric you take to judge this 🙂 mine are: (1) effort to use an arbitrary action even in the most unsafe manner and (2) effort to have some kind of type-safety. In the old world, for (1) one could use
CustomAction
API, and for (2) there is subclassing of
RegularAction
. Thanks to the bindings server, the new lowest-effort method is IMO using the binding provided by the server, and it already gives you maybe not type-safety, but at least the inputs are mapped properly so it will catch certain kinds of errors. Taking this into account, IMO we're getting closer to a something safer by default
How about some itching (...)
I actually really like the idea because it would point the users to contributing to the typing catalog. I want the catalog to be co-owned by the community, and I'll do my best to maintain it (I'm in the middle of creating some tools to make it simpler, e.g. https://github.com/typesafegithub/github-actions-typing-editor). I'll think about what you proposed with the extra suffix!
Also, how do you test your typings?
what do you mean exactly? what kind of test?
v
I'll think about what you proposed with the extra suffix!
Maybe not a good idea, as then the action suddenly gets renamed just because inputs changed. But having the implicitly typed properties deprecated, or the whole action if no typings present, would at least hint to the improvement potential without breaking things.
what do you mean exactly? what kind of test?
Manual. You develop an action or an update for an action. You change also the typings for it. You could maybe there push to some fake tag and use that as dependency to test the action. But pushing for that might not be a good idea, but you might want to test the typings without pushing your action. And even worse, you develop typings for a 3rd party action, either for a PR or for the catalog. You either have to pull a fork and use the fork for testing, or blindly get things merged and test it only after that. Would be better if there were some way to test, for example like I described.
p
> Maybe not a good idea, as then the action suddenly gets renamed just because inputs changed. > I thought about: • generating just the
_Untyped
binding if the action doesn't have any kind of typings provided • generating both the
_Untyped
binding and the type-safe binding if typings are available This way not only we don't break consumers of actions that use the untyped bindings, but also provide a safer way of using an action if some of the inputs have to be defined as GitHub expressions (currently it has to be done using
_customArguments
). Regarding
@Deprecated
, I don't like this approach if we were vending only untyped binding because there's no Kotlin entity immediately available that is not deprecated, so I feel the semantics of deprecation would be broken here. But I like the idea of marking as deprecated the untyped binding if there's also the typed binding. (edit: or actually maybe we shouldn't deprecate it to use it in the się case I described earlier, with the expression)
v
I thought about:
• generating just the
_Untyped
binding if the action doesn't have any kind of typings provided
• generating both the
_Untyped
binding and the type-safe binding if typings are available
The breaking was more for the in-between case when the typing became outdated for example because some input was added, so the
_Partilytyped
case. 😄 But I guess those should maybe just be made deprecated like the whole
_Untyped
class. You could also extend the concept on a propertly level of course, having for each property an
..._Untyped
and if a type was configured one without the suffix. That would also care for misconfigured types as you can then use the
..._Untyped
property until it is fixed.
or actually maybe we shouldn't deprecate it to use it in the się case I described earlier, with the expression
Hm, yeah, if the
_Untyped
concept expands to the property level, that might be enough. Of course the deprecation would conceptually not be the right tool, just was the first that came to mind to notify the consumer that he should do something like making the action author add typings or get them into the catalog repository. The
_Untyped
might be enough as hint. I also want it for me / power users, not only "simple consumers". If I add a new action dependency, I don't want having to look in the action repo and the catalog whether there are typings and indeed everything is
String
. I want some in-workflow-file notification like deprecated or suffix that tells me something should be done eventually. Maybe like this: • Suffix the class and all properties with
_Untyped
if no typing is available • When any typing is available ◦ additionally have the unsuffixed class ◦ maybe mark the suffixed class as deprecated ◦ in the unsuffixed class have all properties with
_Untyped
for not-yet typed things and expressions ◦ additionally add unsuffixed properties for the typed stuff ◦ maybe mark for
String
typed properties the
_Untyped
one as deprecated as also the typed one can be used, or maybe better not in case the type changes in the future, so maybe better always use
_Untyped
for expressions even if the type is
String
Would be better if there were some way to test, for example like I described.
Or you could use a version like
v4__typing-data_base64encodedtypingdata
to supply the typing data that should be used for the generation. That way you could even override typings you found to be wrong, or if you need a field String-typed to use an expression (ignoering the Untyped discussion here 😄)
Btw. actually doing it like that right now would be a breaking change unless you add a new binding server route for the new version. Because if right now already someone uses an untyped action, it will fail with the changes, so it should probably either be done fast and hopefully people are not yet using untyped actions but just translated what they had, or then done in backwards-compatible manner, adding a new binding server version route
p
Ok, I like what you described below "maybe like this", so also with property-level suffixes 😊 I think we don't have that many users of the bindings server yet, so maybe indeed we could implement it in a breaking way, before releasing 3.0 (planned for around September 1st). It depends if I find time to implement the changes by then - if not, let's go with adding a new version of server routes
👌 1
v
Reminds me, that changing typing information also is a breaking change. If for example some type was wrong and gets corrected, for example from
String
to
int
, this will be a breaking change and existing workflows will break as the binding server will generate the new bindings.
p
exactly, that's why ideally the action owner should publish such change as a new major version. If we have a bug just in the typings, then we don't version them separately, so that's a problem. Historically I just fixed them without caring about breaking people, and didn't get a single complaint, probably because the lib is not yet that popular, or I was lucky the action wasn't used 🙂
v
Shouldn't the properties in the untyped class also be untyped? At least that was my intention. If you then switch from untyped to typed, at first things still work and you can change the proprties to typed manually, adjusting the values.
Ah, I see you only named the class up to now, most of my suggestion is not there yet. 😄
p
The intent was to also make the properties untyped (of type String) in the
_Untyped
, isn't it the case? I have a test for it: https://github.com/typesafegithub/github-workflows-kt/blob/9f0928bf719915e1a97bf2e983fad7fd0c55b77c/.github/workflows/test-script-consuming-jit-bindings.main.do-not-compile.kts#L25-L26
If you mean that untyped inputs aren't marked this way in the typed binding, that's right - not sure if i will manage to implement it before releasing v3 (so making the binding server stable), it will probably slip for some time, so we'll have to bump the routes version in the future
v
Maybe I'll find some minutes soon to implement what I envisioned. Wanted to have a look anyway after doing the test-help I created. What I meant was: no typing:
Copy code
class FooAction_Untyped {
   val foo_Untyped: String
   val bar_Untyped: String
}
full typing:
Copy code
@Deprecated
class FooAction_Untyped {
   val foo_Untyped: String
   val bar_Untyped: String
}
class FooAction {
   val foo: Int
   val foo_Untyped: String
   val bar: String
   val bar_Untyped: String
}
partly typed:
Copy code
@Deprecated
class FooAction_Untyped {
   val foo_Untyped: String
   val bar_Untyped: String
}
class FooAction {
   val foo: Int
   val foo_Untyped: String
   val bar_Untyped: String
}
p
Just one thing to clarify: nullability of the fields, I think you wanted to make both a typed and an untyped field nullable, and then check if not both are set at runtime, right?
v
Didn't think about that when making that suggestion, but yes, that's one option as I did it in the typed-input-output-wiring PoC, as there also all fields had an overlay for feeding the expression, so there required check was done at runtime and also that only one of the two are set, like I showed in the snippet I pasted.
Hard to do otherwise as long as we do not have union types if you have two ways to set one required field
But in the typical use-case of these bindings, compilation and execution is done together in most situations anyway, so where the time is spent to do the check is pretty irrelevant performance-wise. It will not be as clear from the IDE where a required type would be non-null, but I think this might be a necessary quirk for both, the untyped-thing and also the typed-outputs-wiring thing.
p
Yeah... There's also a way with using _customArguments, but then one has to set some dummy value of an input is required, so it's not ideal either. Probably even worse than the runtime check. OK, I'm fine with this! But as long as union types arrive, we'll be able to clean it up 🚿😁
👌 1
v
exactly, that's why ideally the action owner should publish such change as a new major version.
And another potential for breaking changes. We have typings in the catalog one way (for example boolean for the tmate action) and the action adds typing that disagrees (for example enum for the tmate action). You can hardly require the tmate action to make a major version bump just for adding typings that disagree what we have in the catalog 😄
Maybe I'll find some minutes soon to implement what I envisioned.
Well, was a bit more than some minutes, but thanks to the previous work on the type-safe output/input wiring, I got it done in full I think. Should be ready to review as soon as gt is re-enabled, as it is on top of my stack so that I could test it in the actual workflow file. 🙂
So we could maybe indeed get this straightened out before 3.0 release 🙂
p
cool! we have Graphite for the next year 🙂
👌 1