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

use githubWorkflowBuildSbtStepPreamble in publish step #501

Closed
wants to merge 1 commit into from

Conversation

bpholt
Copy link
Member

@bpholt bpholt commented May 3, 2023

I think this may have been an oversight.

We don't have to change the default value of sbtStepPreamble, but with this change, the default value is never actually used anymore.

@@ -712,6 +712,7 @@ ${indent(jobs.map(compileJob(_, sbt)).mkString("\n\n"), 1)}
githubWorkflowPublishPreamble.value.toList :::
githubWorkflowPublish.value.toList :::
githubWorkflowPublishPostamble.value.toList,
sbtStepPreamble = githubWorkflowBuildSbtStepPreamble.value.toList,
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to share this setting between the "build" and "publish" jobs. For example, build jobs use this to narrow an axis of the matrix e.g. ++2.13.10 project rootJS, but publishing doesn't want to do this at all. For publishing this should probably just be empty, this seems like a good change for sbt-tl 0.5.

sbtStepPreamble: List[String] = List(s"++ $${{ matrix.scala }}"),
sbtStepPreamble: List[String] = List.empty,
Copy link
Member

Choose a reason for hiding this comment

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

👍 but probably better for 0.5.

@armanbilge armanbilge added this to the v0.5.0 milestone May 4, 2023
@bpholt
Copy link
Member Author

bpholt commented May 4, 2023

Makes sense. I'm going to close this for now; we can retarget and resurrect it if/when it's ready.

@bpholt bpholt closed this May 4, 2023
@armanbilge
Copy link
Member

Thanks, ticketed here:

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

Successfully merging this pull request may close these issues.

2 participants