Introduce AddSecurityScanWorkflow Recipe #36
Conversation
As for getting out of sync with the archetype you might be interested in this recipe that updates the Gradle wrapper, which downloads rather than bundles the corresponding files it needs. For layering changes on top I think indeed manipulating the template with recipes is the way to go.
We have the GitProvenance marker available on every source file. We for instance use that to link SQL statements to the correct GitHub commit hash, but you can similarly extract the ingested branch, which is almost always be the main branch. |
| @Option(displayName = "Build Tool", | ||
| description = "Set up dependency cache.", | ||
| example = "maven", | ||
| valid = {"maven", "gradle"}, | ||
| required = false) | ||
| @Nullable | ||
| String buildTool; |
There was a problem hiding this comment.
Similar to the GitProvenance marker, we also have markers for the BuildTool and LstProvenance. Both of those contain a Type, such that you likely don't have to pass in an argument buildTool here, as that is already determined.
| @Option(displayName = "Java Version", | ||
| description = "Version of Java to set for build.", | ||
| example = "11", | ||
| required = false) | ||
| @Nullable | ||
| Integer javaVersion; |
There was a problem hiding this comment.
In what you might have come to expect by now: we also have a JavaVersion marker that you can use to determine the Java version. You might want to account for projects that use a different version for src/main versus src/test, but other than that it should be convenient not to have to pass this parameter I think.
| return Collections.emptyList(); | ||
| } | ||
| YamlParser parser = new YamlParser(); | ||
| @Language("yml") |
There was a problem hiding this comment.
Could this have been left over from an earlier version where there was an inline String here?
| @Language("yml") |
At least I think; I see this with Kotlin, but not with Java.
src/main/java/org/openrewrite/jenkins/github/AddSecurityScanWorkflow.java
Show resolved
Hide resolved
| boolean licenseHeaderDone = false; | ||
| for (String line : lines) { | ||
| if (licenseHeaderDone) { | ||
| wanted.add(line); | ||
| } else if (line.isEmpty()) { | ||
| licenseHeaderDone = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it make sense to configure the license plugin to the workflow file with something like this?
license {
exclude "**/*.properties"
excludes(["**/*.txt", "**/*.conf"])
}Unless we go for downloading the file on the fly of course, in which case this wouldn't be necessary.
There was a problem hiding this comment.
Nice, thanks! Didn't know this was an option
| * Adapted from {@link org.openrewrite.yaml.AppendToSequenceVisitor} | ||
| */ | ||
| class ReplaceSequenceVisitor extends YamlIsoVisitor<ExecutionContext> { |
There was a problem hiding this comment.
Glad to see you were able to make this work; I'm wondering if it would make sense to pull this up, or change either AppendToSequence or ChangePropertyValue to be able to set a new list of values. Just a thought for now, not something I'd ask of you, but curious how you'd see that. Fine to keep this here for now, and know that it's here if we get similar requests.
There was a problem hiding this comment.
I do think that would make sense. I actually checked both those recipes to see if they already did this.
I think it might make sense for AppendToSequence to have a strategy called REPLACE, but it definitely seems like ChangePropertyValue could take a list of values. I attempted to use ChangePropertyValue with a yaml list [ a, b, c ], but the formatting didn't quite work.
There was a problem hiding this comment.
Nice to see this addition! Some suggestions for alternatives on my end, but you're welcome to resolve those however you see fit.
On a general note I'm not sure if you knew we also have a rewrite-github-actions. I don't think that's applicable for the changes you're doing here, but thought to point it out in case there's more changes you want to do to GitHub Actions files.
What's changed?
Introduced a new recipe to add a GitHub Actions workflow.
What's your motivation?
Closes #22.
Anything in particular you'd like reviewers to focus on?
Are there better patterns for taking an example file and layering changes on top of it? One problem with this implementation is it's possible to get out of sync with the archetype.
The recipe allows specifying branch name, but I think the intent of the workflow is to target the default branch. Is the default branch name (or indexed branch) available in the LST? The implementation would be more useful out of the box if we could read the correct target branch from the LST.
Anyone you would like to review specifically?
@timtebeek
Have you considered any alternatives or workarounds?
ReplaceSequenceVisitorwas adapted fromAddToSequenceVisitorin rewrite-yaml. I think it makes more sense to include it there long-term, but it may be faster to prove out its utility as part of this module. As preparation for an eventual move, it is not public.Rather than specifying the build tool or java version, I thought about inferring both from the environment. Since there are so few Gradle implementations and we already have logic around supported Java versions, I didn't think that was required for a first pass.
Checklist
./gradlew licenseFormat