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

Reconcile with upstream sbt-github-actions #470

Open
armanbilge opened this issue Feb 27, 2023 · 9 comments
Open

Reconcile with upstream sbt-github-actions #470

armanbilge opened this issue Feb 27, 2023 · 9 comments

Comments

@armanbilge
Copy link
Member

https://github.com/sbt/sbt-github-actions

sbt-typelevel made its own fork of sbt-github-actions when it was in a lull, and I was moving fast. Since then @mdedetrich has revived maintenance on the upstream plugin, which is now published under sbt org. The original intent was always to reconcile with upstream, and now the time has come.

For this to be successful, we will need to:

  1. upstream our changes in the fork (several have already been upstreamed)
  2. re-integrate sbt-typelevel with the upstream plugin
  3. have a dedicated maintainer on the upstream plugin, to review PRs in context of how we use the plugin in sbt-typelevel

The benefits will be shared maintenance with the wider community and access to several interesting features happening upstream (e.g., customizable workflow permissions, multiple workflow files, etc.).

@djspiewak
Copy link
Member

djspiewak commented Sep 17, 2023

I took a flyer at this. The delta isn't zero, but it seems low enough that this is tractable. The following APIs need to be upstreamed:

  • githubWorkflowArtifactDownloadExtraKeys
  • WorkflowStep.DependencySubmission (I'm guessing we actually want to keep this for ourselves)
  • SetupJava has an extra boolean parameter?
  • GitHubActionsPlugin.appendtoStepSummary
  • GenerativePlugin.expandMatrix
  • GenerativePlugin.diff (uh… do I want to know?)

We're definitely going to be breaking some compatibility here, but due to the nature of all this stuff, I would imagine the downstream breakage will be minimal.

@armanbilge
Copy link
Member Author

@mdedetrich do you have bandwidth to upstream some of these?

@mdedetrich
Copy link

mdedetrich commented Sep 17, 2023

We're definitely going to be breaking some compatibility here, but due to the nature of all this stuff, I would imagine the downstream breakage will be minimal.

We are not enforcing binary (or even source) compatibility since its pre version 1.0.x and I am not aware of any plugins that extend sbt-github-actions so as long as the breakages are justified it should b fine.

GenerativePlugin.expandMatrix

Is this solving the same problem that sbt/sbt-github-actions#65 is solving?

GenerativePlugin.diff

Yeah this is scaring me

@mdedetrich
Copy link

mdedetrich commented Sep 17, 2023

@mdedetrich do you have bandwidth to upstream some of these?

If you are talking about reviewing + releasing then sure. If you are talking about bisecting the changes to make the PR's that may take a bit of time (will be at a wedding in Georgia and then I have a presentation to prepare for in Canadi early Oct), after that I will have more capacity, but I can try and tackle the low hanging fruit first.

@djspiewak
Copy link
Member

djspiewak commented Sep 17, 2023

@mdedetrich I went through and did some reviewing. I think it's not quite as scary as it looks. Finding so far:

githubWorkflowArtifactDownloadExtraKeys

This is a real thing. Easy feature.

WorkflowStep.DependencySubmission

Similar but even easier. It's a Use

Edit: Actually upon consideration, I kinda think this should stay in sbt-tl. It doesn't need to be in sbt-gha and it's a little TL-specific for my tastes.

SetupJava boolean

Real feature (it's enableCaching defaulted to true). I suspect this is a workaround for some Java installer action BS.

appendtoStepSummary

Actual real feature that kinda looks useful, and it's on the correct object too.

expandMatrix

Seems like a moderately hacky solution but I can't think of a better one right now. I see why it's needed and I think it would benefit from upstreaming. Feels like it could be done more composably but I'm at a loss right now.

diff

Yeah this is a hack. It supports the MergifyPlugin. We probably just need to move this down there and eat the duplication.

@mdedetrich
Copy link

@djspiewak @armanbilge So I went ahead and already cherry picked one of the easier/low hanging fruit features githubWorkflowArtifactDownloadExtraKeys, see sbt/sbt-github-actions#165

@djspiewak
Copy link
Member

@mdedetrich any opinions on DependencySubmission (upstream or nah)? I'm not fussed much either way. I believe Arman's view is that it would be useful in the upstream.

@mdedetrich
Copy link

Ill have a deeper look into it tomorrow, I just picked the first easy one on the list for today

@mdedetrich
Copy link

@djspiewak @armanbilge FYI on a related note, I am planning on making a release of sbt-github-actions after sbt/sbt-github-actions#166 is merged. This is because some of the changes here are on the more extensive side so I want to make a release before we start merging these upstream PR's so that not everything is released at once "big bang" style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants