-
Notifications
You must be signed in to change notification settings - Fork 61
Refer to actions by commit hash #196
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,17 +33,31 @@ object WorkflowStep { | |
val DefaultSbtStepPreamble: List[String] = List(s"++ $${{ matrix.scala }}") | ||
|
||
val CheckoutFull: WorkflowStep = Use( | ||
UseRef.Public("actions", "checkout", "v5"), | ||
UseRef.Public( | ||
"actions", | ||
"checkout", | ||
"08c6903cd8c0fde910a37f88322edcfb5dd907a8", | ||
"v5.0.0"), | ||
Comment on lines
-36
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sympathetic to having an immutable and repeatable CI configuration, but the downside to embedding git sha into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jup - this PR was more of a 'straw man' to get the conversation going than something I'd expect to get merged in this form. What would be the way forward here? Perhaps we could have an option that makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the limited number of upstream actions, we could probably run some weekly cron job that check them out and maintain a JSON file of action, version, and gitsha, maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could, but that'd kind of pressure us into releasing a new version of sbt-github-actions each time that job finds something to update - I'm not sure we'd want that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea so even in the JSON metadata world maybe it's better to default to v4 style and let the user opt into git shas. That way for the most users we won't be the bottleneck to release the patches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we could keep using tags/branches but 'accept' the hashes in the check, like #207 ? |
||
name = Some("Checkout current branch (full)"), | ||
params = Map("fetch-depth" -> "0")) | ||
|
||
val Checkout: WorkflowStep = Use(UseRef.Public("actions", "checkout", "v5"), name = Some("Checkout current branch (fast)")) | ||
val Checkout: WorkflowStep = Use( | ||
UseRef.Public( | ||
"actions", | ||
"checkout", | ||
"08c6903cd8c0fde910a37f88322edcfb5dd907a8", | ||
"v5.0.0"), | ||
name = Some("Checkout current branch (fast)")) | ||
|
||
def SetupJava(versions: List[JavaSpec]): List[WorkflowStep] = | ||
versions map { | ||
case jv @ JavaSpec(JavaSpec.Distribution.GraalVM(Graalvm.Version(graalVersion)), version) => | ||
WorkflowStep.Use( | ||
UseRef.Public("graalvm", "setup-graalvm", "v1"), | ||
UseRef.Public( | ||
"graalvm", | ||
"setup-graalvm", | ||
"2a2412009026a83f51d179f92dc2b3fd4c8142df", | ||
"v1.4.1"), | ||
name = Some(s"Setup GraalVM (${jv.render})"), | ||
cond = Some(s"matrix.java == '${jv.render}'"), | ||
params = ListMap( | ||
|
@@ -54,7 +68,11 @@ object WorkflowStep { | |
"cache" -> "sbt")) | ||
case jv @ JavaSpec(JavaSpec.Distribution.GraalVM(Graalvm.Distribution(distribution)), version) => | ||
WorkflowStep.Use( | ||
UseRef.Public("graalvm", "setup-graalvm", "v1"), | ||
UseRef.Public( | ||
"graalvm", | ||
"setup-graalvm", | ||
"2a2412009026a83f51d179f92dc2b3fd4c8142df", | ||
"v1.4.1"), | ||
name = Some(s"Setup GraalVM (${jv.render})"), | ||
cond = Some(s"matrix.java == '${jv.render}'"), | ||
params = ListMap( | ||
|
@@ -65,7 +83,11 @@ object WorkflowStep { | |
"cache" -> "sbt")) | ||
case jv @ JavaSpec(dist, version) => | ||
WorkflowStep.Use( | ||
UseRef.Public("actions", "setup-java", "v5"), | ||
UseRef.Public( | ||
"actions", | ||
"setup-java", | ||
"dded0888837ed1f317902acf8a20df0ad188d165", | ||
"v5.0.0"), | ||
name = Some(s"Setup Java (${jv.render})"), | ||
cond = Some(s"matrix.java == '${jv.render}'"), | ||
params = ListMap( | ||
|
@@ -76,15 +98,24 @@ object WorkflowStep { | |
|
||
def SetupSbt(runnerVersion: Option[String] = None): WorkflowStep = | ||
Use( | ||
ref = UseRef.Public("sbt", "setup-sbt", "v1"), | ||
ref = UseRef.Public( | ||
"sbt", | ||
"setup-sbt", | ||
"3e125ece5c3e5248e18da9ed8d2cce3d335ec8dd", | ||
"v1.1.14"), | ||
params = runnerVersion match { | ||
case Some(v) => Map("sbt-runner-version" -> v) | ||
case None => Map() | ||
}, | ||
name = Some("Setup sbt"), | ||
) | ||
|
||
val Tmate: WorkflowStep = Use(UseRef.Public("mxschmitt", "action-tmate", "v2"), name = Some("Setup tmate session")) | ||
val Tmate: WorkflowStep = Use( | ||
UseRef.Public( | ||
"mxschmitt", | ||
"action-tmate", | ||
"ece3d66d6d54a01594acd0ee2e79d1bfb2df136d", | ||
"v2"), name = Some("Setup tmate session")) | ||
|
||
def ComputeVar(name: String, cmd: String): WorkflowStep = | ||
Run( | ||
|
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.
Seeing as
ref
is now always rendered as a comment, I think it makes to keepref
as the actual reference used (be it a hash, tag, or branch name), and instead introducecomment: Option[String] = None
. This way we'd retain source compatibility, also allowing users to opt-in to reference actions by a hash + comment, if they aren't already.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.
I'm not sure - I think it's actually a feature that users would have to review the call site to make sure the third argument is a
rev
. I could see making the fourth parameter acomment: Option[String]
though, no strong preference.