-
Notifications
You must be signed in to change notification settings - Fork 61
Concurrency and reusable workflows #206
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
base: main
Are you sure you want to change the base?
Concurrency and reusable workflows #206
Conversation
fac5434 to
3d8cba8
Compare
| final case class Steps( | ||
| id: String, | ||
| name: String, | ||
| steps: List[WorkflowStep], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling an ordinary WorkflowJob with concrete steps as Steps I think is misleading what's specified here is a job, not the steps.
I suggesting keeping WorkflowJob as-is, and maybe name the abstract class as AbstractJob, and the new entity as WorkflowApply, since hopefully many people are familiar with calling function calls as apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not super happy about the naming either. I chose this to mimic how individual steps are modeled in WorkflowStep.scala, where WorkflowStep is a sealed trait with the Run, Sbt, and Use implementations. Continuing that pattern, a WorkflowJob then would have two different implementations: one which is composed of steps, and then WorkflowApply.
Another alternative I just thought of is to keep WorkflowJob as a case class and instead introduce a sealed trait WorkflowAction which has the WorkflowApply and WorkflowSteps (naming TBD) implementations. An auxiliary factory apply function could be used to keep source compatibility.
If we introduce AbstractJob there's the potential of source incompatibility since some of the keys has to change. githubWorkflowAddedJobs becomes settingKey[Seq[AbstractJob]]. But maybe I'm being too cautious..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative I just thought of is to keep
WorkflowJobas a case class and instead introduce asealed trait WorkflowActionwhich has theWorkflowApplyandWorkflowSteps(naming TBD) implementations. An auxiliary factoryapplyfunction could be used to keep source compatibility.
I'm +1 with that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eed3si9n I've updated the PR to introduce said trait. Unfortunately it's not possible to use argument overloading in conjunction with default arguments on the apply function in WorkflowJob to suit both the "steps" and "apply" use cases. As before, the default behavior is to use "steps", to be source compatible. PTAL.
3d8cba8 to
4dd08af
Compare
This PR adds support for calling reusable workflows instead of listing
stepswithin a workflow. Since there now exist a difference in the set of supported keys between a "standard" workflow job (which uses steps) and a calling workflow variant, theWorkflowJobtrait is made into ansealed abstract classwith two implementations:StepsandUse, with a slight difference in required and accepted arguments. To stay source compatible, theWorkflowJob.applyfunction is aliased toWorkflowJob.Stepsto avoid build definitions using this plugin to change.The
concurrencykeyword is disabled by default, I didn't really dare make a change which would require more or less all workflow to be regenerated (which I guess shouldn't be a problem as scala steward takes care of that), but I'm happy to change it to fall in line with what sbt-typelevel-github-actions does (or what is discussed in #79). The concurrency implementation is essentially a backport from the typelevel fork.Resolves #79