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

Draft design for the matrix context? #532

Closed
wants to merge 4 commits into from
Closed

Conversation

jmfayard
Copy link
Collaborator

See #368 and #297

This is just a draft of what the API looks like.
I didn't try to generate the YAML correctly, and there are some features of the matrix that I don't really understand, like having objects, include (not supported) and excludes

wdyt?

I can't run the project for now because of a timeout
> Could not download binary-compatibility-validator-0.12.1.jar (org.jetbrains.kotlinx:binary-compatibility-validator:0.12.1)
does that ring a bell? https://scans.gradle.com/s/ymfeu563hrxfc/failure#1
on = listOf(Push()),
sourceFile = Path.of("some_workflow.main.kts"),
) {
val matrix = object : StrategyMatrix() {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered making the strategy one of workflow's arguments, like it's done now in the simplified implementation?

Copy link
Collaborator Author

@jmfayard jmfayard Oct 31, 2022

Choose a reason for hiding this comment

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

to be honest, I am not a fan that workflow(), job() have like 10 parameters each already
some could be part of the JobBuilder /WorkflowBuilder DSL instead IMHO

Copy link
Member

Choose a reason for hiding this comment

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

These approaches have both pros and cons. Some of the pros of the argument approach I see:

  • better discoverability in the IDE - the users are more likely to discover an argument than some extra DSL piece
  • compile-time enforcement of having this matrix defined just once in a well-defined, canonical place

One of the cons is sure the number of arguments which starts to become hard to maintain.

Let's defer this decision until we have a better understanding on the features we want to implement.

return Property(value.map { it.toString() })
}

public fun objects(vararg value: Map<String, String>): Property {
Copy link
Member

Choose a reason for hiding this comment

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

The objects can have also non-string attribute values.

@krzema12
Copy link
Member

@jmfayard thanks for starting working on it!

IMO first we should enumerate all features of GitHub strategies, understand them well (with some examples in YAML), decide which ones we want to implement (let's start with "all"), and then propose examples how each of them is covered in the DSL. Does it make sense?

@jmfayard
Copy link
Collaborator Author

yes absolutely, that's also what I had in moind.
Do a first proof of concept of what a type-safe api could look like,
then go back to understand the corner cases of what the strategy matrix supports

@krzema12
Copy link
Member

krzema12 commented Oct 31, 2022

@jmfayard I'd also love to engage @Vampire into this design, so that he can play with some intermediate version of the API in the context of his workflows (some of them make heavy use of the matrices).
@Vampire FTR we're not there yet, will let you know :)

@krzema12
Copy link
Member

@jmfayard are you planning to work on this, or you're fine with me taking over the task? I'm drafting the API in #635

@krzema12
Copy link
Member

Please reopen if needed.

@krzema12 krzema12 closed this Jan 23, 2023
@krzema12 krzema12 deleted the 368-matrix-context branch January 23, 2023 09:18
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.

None yet

2 participants