-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add sbt-github-actions for basic CI #14
Conversation
@@ -8,3 +8,15 @@ pekkoParadoxGithub := Some("https://github.com/apache/incubator-pekko-site") | |||
inConfig(Assets)(Seq( | |||
excludeFilter := excludeFilter.value -- ".htaccess" | |||
)) | |||
|
|||
ThisBuild / githubWorkflowTargetTags ++= Seq("v*") | |||
ThisBuild / githubWorkflowPublishTargetBranches := Seq() |
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.
Intentionally left blank, lets not worry about publishing for now.
86172c8
to
a7d85c6
Compare
|
||
name: Continuous Integration | ||
|
||
on: |
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.
the lack of a permissions declaration is a problem - it would be great if the sbt plugin supported setting this
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.
It does now, I just forgot to set it. By default its full permissions.
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.
Do you have a suggestion for the permissions?
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.
if the GitHub Token is not removed, then this is probably what we need
permissions:
contents: read
tags: [v*] | ||
|
||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
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.
why do we need this? this token appears to be unused in this CI job
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.
Its there by default but I might be able to turn it off.
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.
So from https://github.com/sbt/sbt-github-actions#generative-1
githubWorkflowEnv
:Map[String, String]
– An environment which is global to the entire ci.yml workflow. Defaults toMap("GITHUB_TOKEN" -> "${{ secrets.GITHUB_TOKEN }}")
since it's so commonly needed.
So to me the reasoning behind this is sensible considering. I am not entirely 100% sure if the current actions that are created by sbt-github-actions don't require a token, if you are sure of that I can remove it otherwise as a compromise we can leave it as it is and try to remove it in a future PR (for some reason github actions aren't triggering right now).
@pjfanning BTW If you have any general questions/issues regarding sbt-github-actions you can report it upstream at https://github.com/sbt/sbt-github-actions, I am actually a maintainer of the project. |
strategy: | ||
matrix: | ||
os: [ubuntu-latest] | ||
scala: [2.12.18] |
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.
scala: [2.12.18] | |
scala: [2.12] |
Btw. why is it running on 2.12 in the first place?
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.
Its a sbt plugin
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.
Also regarding including the patch version (i.e. 18
), this is automatically generated by sbt-github-actions. The full Scala version is not necessary anymore in later sbt versions so this can be improved, see sbt/sbt-github-actions#158
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.
No need for this in my opinion, it adds more complexity than necessary right now.
Closing this since there isn't willingless to merge it |
@jrudolph Incase you change your mind, there was additional context for this PR (see #13 (comment)). Sorry if it wasn't clear from the beginning. |
@mdedetrich I don't object to this PR - I think it is useful to run We should probably even be thinking about a link validator job. |
Sure, if @jrudolph changes his mind I will re-open the PR but this is low priority for now |
So that we at least don't accidentally break something.