Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add action typing declaration #30

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

krzema12
Copy link
Contributor

@krzema12 krzema12 commented Jun 22, 2022

Pilot for typesafegithub/github-workflows-kt#302

Not necessarily for merging as is, but as a starting point for further discussion
and another round of feedback.

action-types.yml Outdated
@@ -0,0 +1,35 @@
typingSpec: krzema12/[email protected]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary?
Isn't the custom file already sign enough that it is following the github-actions-typing spec?
If the spec evolves in a backwards incompatible way, then maybe a simple version key or similar would be enough?
And as long as only backwards compatible changes are done, just assuming a default version should be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for bringing it up, I keep thinking if it's a good idea to have this value. The idea is that as long github-actions-typing is at v0.x, breaking changes may happen (e.g. I'll change casing according to your suggestion). By having this field I wanted the consumers to be able to depend on v0 (not pin to minor version) and the action would know how to validate all versions we had along the way.

If you, as an early adopter, are fine with occasional breaking changes and a need to bump typing validation action, then I'm fine with what you propose. Once we consider the typing spec stable (v1), version key won't yet be needed, and we could postpone introducing it until v2 if it has breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you, as an early adopter, are fine with occasional breaking changes and a need to bump typing validation action

I guess I can live with that, especially as I don't expect too many v0 breaking changes, and I'm sure you will create PRs for them. :-)

action-types.yml Outdated
Comment on lines 24 to 25
listItem:
type: string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you expect more sub-keys for listItem?
Otherwise maybe just a listItemType key, or list-item-type, or similar directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there can be more, e.g. for an enum (see here) or an integer with named values. In terms of typing, whatever goes under listItem behaves similarly like action's input/output. The only limitation for now is that list items cannot themselves be lists - I want to keep it simple for now. No use case for List<List<List<String>>> appeared yet 😉

Copy link

@jmfayard jmfayard Jun 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No use case for List<Boolean> either :)
imagine - enable: [true, false]
or - enable: [true, true]
or - enable: []

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we could simplify it further and allow lists of only integers and enums - I wanted to do it initially. However, then I asked myself what if we allow basic types there, and forbid only nesting lists because then it may become more tricky. So allowing e.g. List<Boolean> stems from the idea of supporting lists of basic types, not having a real-life use case for it :) Would you guys recommend limiting lists only to lists of strings and enums? We can always extend it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, this discussion doesn't seem to be relevant for the decision if there should be list-item: with subkeys or list-item-type: because the enum kinda requires the former.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list-item feels better to me

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, yeah, if you need more subkeys potentially like for enums, list-item is fine of course.
I think I'd not limit the subtype (further than forbidding lists unless someone comes up with a use-case).
If someone wants a list of booleans, why not, if you don't need special code to support it and I guess you don't , or at least not significant.

action-types.yml Outdated
inputs:
distribution:
type: enum
allowedValues:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard keys in action and workflow files follow kebab-case,
maybe this file should also use it instead of camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub is not very consistent when it comes to casing - apart form what you wrote, I've seen snake_case as well. But you're right that kebab-case is the most frequent casing, let me change it in the whole typing spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually they even have one case I have seen where they used camelCase, for deprecationMessage. :-D
But yeah, most is kebab-case, that's what I meant. :-)

@krzema12
Copy link
Contributor Author

Thanks for great hints! I applied them in 6d17d07, but without necessary changes in the validation action yet. Will wait for a "ship it" from your side, then will adjust the action, and then we can rerun the checks and merge.

@Vampire
Copy link
Owner

Vampire commented Jun 26, 2022

Will wait for a "ship it" from your side, then will adjust the action, and then we can rerun the checks and merge.

I think it looks good to me spec-wise.
I'll probably fiddle a bit about the formatting like sprinkling in some newlines and remove the quotes of strings where not necessary, but I can do that to my liking, no need to micro-manage there. :-)

Btw. do you use a parser that allows usage of YAML anchors?
It might happen that someone defines a list of possible enum values and then later has onther input with a list of the same enum values, so it would be nice if the validation action and the consuming code would be able to use YAML anchors, unlike GitHub actions themselves.
Another good reason to have it in a separate file. :-)

@krzema12
Copy link
Contributor Author

Btw. do you use a parser that allows usage of YAML anchors?

I use kaml. According to this, it supports anchors. I haven't tested it yet, though. Thanks for the hint!

@Vampire
Copy link
Owner

Vampire commented Jun 26, 2022

Yes, kaml fully supports anchors.
Maybe you remember I told you I use kaml currently to parse my workflow template with extensive anchor usage and then again to write it out to have it flattened out like GitHub Actions requires it. :-)

Would be nice if that is a documented guarantee in the spec and not just an accidental implementation detail. :-)
This way we can nail you on it if you happen to change to something not supporting them. :-D

@krzema12
Copy link
Contributor Author

Would be nice if that is a documented guarantee in the spec and not just an accidental implementation detail. :-)
This way we can nail you on it if you happen to change to something not supporting them. :-D

Got it, sure thing: typesafegithub/github-actions-typing#12

@krzema12 krzema12 force-pushed the add-action-typing-declaration2 branch from 772a993 to e41e295 Compare June 28, 2022 21:11
@krzema12
Copy link
Contributor Author

krzema12 commented Jun 28, 2022

@Vampire I've adjusted the action - feel free to restart the workflows.

Note: please don't set the verbose flag, I'll remove it soon in favor of debug logging you proposed.

@krzema12
Copy link
Contributor Author

Yay, works!
image

Co-authored-by: Björn Kautler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants