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

Feature/workflow support #65

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

987Nabil
Copy link

@987Nabil 987Nabil commented Jun 1, 2021

Impl. #7

@987Nabil
Copy link
Author

987Nabil commented Jun 1, 2021

@djspiewak
I do not want flying to mars and then realising we just want to go to the moon.
So could you take a look and tell me, if I am in the right space ship? 🚀 😃

@djspiewak
Copy link
Collaborator

This is a really nice start! Definitely headed in the right direction, though I have some aesthetic quibbles with file organization. :-P

@987Nabil
Copy link
Author

987Nabil commented Jun 1, 2021

Still wip ;)

I realised until now, you have the data model separated from the rendering logic. Would you like to keep this?
Or you think my mixed approach is ok too?

As you can see, I copied some useful methods from GenerativePlugin. Do you have any suggestion where to put such code for usage in both files?

My gut feeling says, that GenerativePlugin is quite big.
Is working on this topic maybe a good place to rethink the current design and maybe split the file a bit? 🤔

@987Nabil
Copy link
Author

987Nabil commented Jun 3, 2021

For simplification, I plan not to implement branches-ignore or tags-ignore for events that would support it.
Since one can not mix it with branches and tags, but there is the negation syntax !<branchPattern>.

You can exclude tags and branches using the ! character. The order that you define patterns matters.

A matching negative pattern (prefixed with !) after a positive match will exclude the Git ref.
A matching positive pattern after a negative match will include the Git ref again.

I'll add a part about it in the docs.
See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-using-positive-and-negative-patterns

- All TriggerEvents have now a corresponding case class/object
- Add new sbt key githubWorkflowCustomWorkflows for custom workflows
- Generate custom workflows within githubWorkflowGenerate
@987Nabil 987Nabil marked this pull request as ready for review June 7, 2021 12:21
@987Nabil
Copy link
Author

987Nabil commented Jun 7, 2021

@djspiewak I think the model should fit now, as well as the generation.
I tried to go with a minimal code change approach. I also don't think this changes should be breaking in any means, since the usage of Workflow in the old logic is not reachable from the public, if I see this correctly.

Imo the current state should be fine for an initial release of the feature.

@987Nabil
Copy link
Author

987Nabil commented Jun 8, 2021

Open questions:

  • githubWorkflow settings like the Scala version, OSs or the definition for runs-on are the same for all jobs. Should that be customisable?
  • Should there be more validation? Cron expressions for the Schedule trigger event for example?

The os field in build matrices are is not mandatory.
On private action runners you may just want to provide custom labels.
This is now possible.
When having custom workflows, one might not want to have ci.yml or
clean.yml generated. This is now configurable via
githubWorkflowGenerationTargets
@987Nabil
Copy link
Author

@djspiewak I am using this version currently in multiple projects and it works well so far. I changed to the way oses are compiled and added the GenerationTarget since I realised I don't want to have CI and Custom workflows.
But I think the current solution should be flexible enough for all needs.

@catostrophe
Copy link

@987Nabil does this support adding the workflow_dispatch event as a trigger?

@987Nabil
Copy link
Author

@catostrophe For custom workflows yes. For the default CI job aka ci.yml no. push and pull_request are currently still hard coded for the ci job.
It is in this state, since the intention is not to break any old code.

@catostrophe
Copy link

catostrophe commented Jun 22, 2021

@987Nabil that's exactly what I need - to tweak the default ci.yml to run on workflow_dispatch event. I wanted to add this as a small PR via a flag

@djspiewak what's the best way to introduce this feature? As a separate small PR or as a part of this one?

@987Nabil
Copy link
Author

@catostrophe my suggestion would be: Merge this PR first, then do it separately afterward. I think this PR is quite big already. Even though I guess it might be a small change.

…pport

# Conflicts:
#	src/main/scala/sbtghactions/GenerativePlugin.scala
@987Nabil
Copy link
Author

@djspiewak I just merged the main. And I am already heavily using it with a local build. Would be nice if you could review this and I could use an official build :)

@djspiewak
Copy link
Collaborator

@987Nabil Sorry for the delay! Looking at this now

@robmwalsh
Copy link

It would be great to have this revived/merged!

@jdegoes
Copy link

jdegoes commented Jan 30, 2023

I would love to see this pull request merged in! ❤️

@eed3si9n
Copy link
Member

@987Nabil

But since this is living now in the SBT org, I thought I ask you, if you are fine with me reviving this pr? If so, it would be great if you can comment on the general structure of the or, so i could then go on to fix/update it 🙂

I am here as a backstop, but generally the maintainers are @sbt/sbt-github-actions team.
If there are no breaking changes to the existing builds, I think the PR looks pretty good already. Maybe it should be better documented in README?

@mdedetrich
Copy link
Contributor

mdedetrich commented Jan 30, 2023

@987Nabil I can have a look at it, but to get the premise of the PR by custom workflow you mean the ability to generate a completely custom github action workflow flow with the various actions.

If so I can try to look at it during in a week (I have a conference to go to). In the meantime if you could rebase the PR to fix the current merge conflicts that would be great!

Also agree with @eed3si9n regarding the documentation

@987Nabil
Copy link
Author

987Nabil commented Jan 30, 2023

@eed3si9n sorry, I just saw SBT org and you changing something, so I assumed 😅

@mdedetrich basically it aims to be a scala DSL for actions/workflows. And yes, the goal is to not change any api but just extend it.
I think the origin of the plugin was focused on OSS builds. But companies need maybe more complex CI/CD pipelines with more customizable jobs/steps and would benefit from having reusable code building blocks.

@mdedetrich
Copy link
Contributor

mdedetrich commented Jan 31, 2023

@987Nabil Thanks for the response, as mentioned before I will realistically look at it from next week Monday but if I can find some time I might start earlier.

I suspect that the reason behind the merge conflicts is the permissions feature at #105 which I wrote and was consequently merged. On the surface I think it actually makes sense to incorporate that feature into your DSL when doing the rebase.

@987Nabil
Copy link
Author

@mdedetrich there where also changes to the GitHub api. So I'd like to update anyway. I'll check on it today or maybe tomorrow.

@mdedetrich
Copy link
Contributor

Perfect thanks!

@987Nabil
Copy link
Author

987Nabil commented Feb 3, 2023

@mdedetrich Sorry it might take longer than I expected. Maybe I can find some time today.

…pport

# Conflicts:
#	src/main/scala/sbtghactions/GenerativePlugin.scala
#	src/test/scala/sbtghactions/GenerativePluginSpec.scala
@mdedetrich
Copy link
Contributor

No worries, no rush

@987Nabil
Copy link
Author

987Nabil commented Feb 3, 2023

@mdedetrich I guess this is now in a reviewable state. The branch is up to date and I migrated the permissions. I simplified and generalized some code a bit. This comes with a little less beautiful workflows (see the missing empty lines), but is an overall benefit I guess, also in terms of maintainability.
I am happy to add more docu, if you can give me a hint what you would expect.
Should I add an example?

Also I did not check, if this state on the same feature level as the latest github api.
But I guess it is ok to add stuff later if missing. The PR is already large.

@mdedetrich
Copy link
Contributor

@987Nabil Thanks, I will start looking at it in a few days. Note that some of the scripted tests are still failing

@@ -617,16 +617,6 @@ ${indentOnce(workflow.jobs.map(compileJob(_, sbt)).mkString("\n\n"))}
} else {
githubWorkflowSbtCommand.value
}
//compileWorkflow(
Copy link
Contributor

Choose a reason for hiding this comment

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

@987Nabil What is the context behind this test being commented, i.e. is it temporary to pass CI or is it intentional?

Copy link
Author

Choose a reason for hiding this comment

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

It was only an example I copied for how the current call looks like. I forgot to remove it.

Copy link
Author

Choose a reason for hiding this comment

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

I just realised it is already gone.

@mdedetrich
Copy link
Contributor

Reviewing this today

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Overall very nice work. I just have some minor comments (apologies for the spam regarding documentation for the DSL, its just easier to track).

I do have one question regarding how githubWorkflowJavaVersions/githubWorkflowOSes/ interact with this PR?

*/

package sbtghactions

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add documentation with the @see to the equivalent github documentation (incase something changes/breaks), see https://github.com/sbt/sbt-github-actions/blob/main/src/main/scala/sbtghactions/PermissionScope.scala#L21-L23 for an example

*/

package sbtghactions

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add documentation with the @see to the equivalent github documentation (incase something changes/breaks), see https://github.com/sbt/sbt-github-actions/blob/main/src/main/scala/sbtghactions/PermissionScope.scala#L21-L23 for an example

*/

package sbtghactions

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add documentation with the @see to the equivalent github documentation (incase something changes/breaks), see https://github.com/sbt/sbt-github-actions/blob/main/src/main/scala/sbtghactions/PermissionScope.scala#L21-L23 for an example

*/

package sbtghactions

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add documentation with the @see to the equivalent github documentation (incase something changes/breaks), see https://github.com/sbt/sbt-github-actions/blob/main/src/main/scala/sbtghactions/PermissionScope.scala#L21-L23 for an example

@@ -115,6 +116,7 @@ Any and all settings which affect the behavior of the generative plugin should b
- `githubWorkflowJobSetup` : `Seq[WorkflowStep]` – The automatically-generated checkout, setup, and cache steps which are common to all jobs which touch the build (default: autogenerated)
- `githubWorkflowEnv` : `Map[String, String]` – An environment which is global to the entire **ci.yml** workflow. Defaults to `Map("GITHUB_TOKEN" -> "${{ secrets.GITHUB_TOKEN }}")` since it's so commonly needed.
- `githubWorkflowAddedJobs` : `Seq[WorkflowJob]` – A convenience mechanism for adding extra custom jobs to the **ci.yml** workflow (though you can also do this by modifying `githubWorkflowGeneratedCI`). Defaults to empty.
- `githubWorkflowCustomWorkflows`: `Map[String, Workflow]` - This is the place to define your custom workflows. The key represents the filename (**.yml** is added if not provided) the value the workflow definition. The settings of the generative plugin apply, as long as they are not part of the workflow trigger events. For example, the `githubWorkflowTargetBranches` setting has no influence on custom workflows, but `githubWorkflowScalaVersions` does.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about keys like githubWorkflowJavaVersions or githubWorkflowOSes? Is this supported (if not I suspect that this may be a case of feature creep but having the ability to interpolate these keys into custom workflows would be useful/beneficial if practical).

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what you have in mind, but this is now a very general API. It might be, that there is not even a java version needed for the workflow. Also GitHub has the feature of self hosted runners, that might not have the tags for the oses. So that might be confusing for the user. I guess using this keys should be up to the one defining the Workflows.

Copy link
Contributor

@mdedetrich mdedetrich Feb 25, 2023

Choose a reason for hiding this comment

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

So to put it differently, what I had is having the ability to use the githubWorkflowJavaVersions/githubWorkflowOSes values in a custom ReusableWorkflowJob. While its definitely true that it doesn't make sense in some cases, at least having access to those values can be useful for certain types of custom workflows which this PR implements as a feature.

What I had in mind is adjusting ReusableWorkflowJob so that it has the https://github.com/sbt/sbt-github-actions/pull/65/files#diff-535533c663b9848aff0c0b943149d3dd8b126b34aa463e507ada7e6011ce8288R38-R39 so that it has the scalas and javas variables.

See https://github.com/sbt/sbt-github-actions/pull/65/files#r1117907905.

@mdedetrich
Copy link
Contributor

@987Nabil Did you have time to look into the review comments? Also @armanbilge incase you have time to review the PR yourself.

@987Nabil
Copy link
Author

Sorry @mdedetrich for some reason I did not get notified and I just saw it. I'll check it soon.

name: String,
uses: WorkflowRef,
cond: Option[String] = None,
needs: List[String] = List(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add javas and scalas here (like with WorkflowJob) so that these values can be accessable if someone wants to create a custom workflow that uses those values?

Copy link
Author

Choose a reason for hiding this comment

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

I am not so sure about that. I think you can make a very good argument, that it is after this PR more or less a smell, that WorkflowJob does not just represent the GH api and that as a pure data structure should only do that.
And yes, I bring this argument, because I think it is the better design. What I did not think about yet is, if this would bing migration issues.
But I think, in a perfect world there would be no oses, javas and scalas here, but the plugin would build the right WorkflowJob out of the sbt keys before rendering it.

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about it, I think it does make sense in a future PR because as you said some things might need to be moved around and this PR has already been open for some time. The reason I brought this up is because this is actually a usecase for one of the projects I maintain, i.e. we definitely have a use for custom workflows however we are still going to be running jobs on JVM's defined by githubWorkflowJavaVersions and OS's defined by githubWorkflowOSes (amongst other things) and without such a change you basically have to redefine the exact same values (once in githubWorkflowJavaVersions and another in your custom workflow) which also has the obvious risk of them accidentally going out of sync.

Copy link
Author

@987Nabil 987Nabil Feb 26, 2023

Choose a reason for hiding this comment

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

You actually don't need to have any risk. You will define the workflow anyway in an sbt file or your own sbt plugin. And there you can know the key. You can just use it somewhat like this.

  val SetupScala: WorkflowStep.Use = WorkflowStep.Use(
    name = Some("Setup Java and Scala"),
    ref = UseRef.Public("coursier", "setup-action", "v1"),
    params = Map("jvm" -> GenerativeKeys.githubWorkflowJavaVersions.value.head.version),
    )

What I could understand is, having a method on the data types like withJavaVersions that sets a matrix up in the right way. But I would really remove every data from the structure that is not a reflection of the github api.

If you like, we could try to have a call and try to talk about a strategy for this or pairing on a follow up issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you like, we could try to have a call and try to talk about a strategy for this or pairing on a follow up issue.

Sure! Next week I will have some time (feel free to email me to discuss, its on my profile)

For now I will merge the PR in a few days to see if we can get any more additional reviews.

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Approving PR

@mdedetrich
Copy link
Contributor

@armanbilge I will wait till mid next week if you have a few comments, at which point I will merge.

@987Nabil
Copy link
Author

@mdedetrich Is there something blocking this except the merge conflicts? If not I'd solve this asap to merge it.

@mdedetrich
Copy link
Contributor

mdedetrich commented Mar 21, 2023

@987Nabil

@mdedetrich Is there something blocking this except the merge conflicts? If not I'd solve this asap to merge it.

So I want to give some context behind whats going on, I spoke with @armanbilge and currently right now there is an issue where this project needs another active maintainer that actually uses this project (which is as of now is just me). Due to this I decided to try and make a 0.2.0 release with bug fixes/minor feature improvements that have been collected in this repository over time and have not been released yet. The reason why I want to get 0.2.0 out as fast as I can is because the current latest version of sbt-github-actions is referencing old versions of github actions (such as the cache action) that is generating warnings for projects (and there are other fixes as well).

Once 0.2.0 is released I want to properly look into this PR, which also means trying to use it in one of the projects that I maintain (likely one of the Pekko ones) to see if anything is missed since this feature is directly relevant and very useful for me specifically. It would have been ideal to get another set of eyes from an active maintainer on this feature change (which is what I originally desired) but as mentioned before this isn't going to happen anytime soon hence why I want to delay this for 0.3.0 and not have this change specifically block all the other changes.

In regards to merge conflicts, there will be one more additional non trivial PR merge which is #136. After that point I will upstream some of the relevant bug fixes in typelevels fork of sbt-github-actions (they ended up forking this project because it wasn't maintained) which may generate more conflicts.

If you have the time to merge the conflicts than that would be great, otherwise I can also just push the merge conflicts on your branch as the mentioned changes start rolling in.

@987Nabil
Copy link
Author

@mdedetrich I want to check on what the status here. Do you see a chance to get this in somehow? I am willing to fix things, but want to make sure I don't work on something that can't succeed :)

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.

7 participants