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

Allow custom YAML filenames in YamlParser #4062

Closed
jimschubert opened this issue Mar 1, 2024 · 4 comments · May be fixed by #3993
Closed

Allow custom YAML filenames in YamlParser #4062

jimschubert opened this issue Mar 1, 2024 · 4 comments · May be fixed by #3993
Labels
enhancement New feature or request

Comments

@jimschubert
Copy link

What problem are you trying to solve?

My company has an internally built CI/CD pipeline which is configured by extensionless YAML files. It's not uncommon for use to have tests with an extension of the full filename. For example, we may have the following YAML documents

MyFile
path/to/testcase.myfile

The yaml extensions are hard-coded in the parser. See:

return fileName.endsWith(".yml") || fileName.endsWith(".yaml");

This causes plugins to not accept our files as YAML. See: https://github.com/openrewrite/rewrite-maven-plugin/blob/ca7c52a91cbe15ef55a4825f0cde2bbc04588164/src/main/java/org/openrewrite/maven/ResourceParser.java#L174

Describe the solution you'd like

I'd like to see a system property exposed which allows defining specific file patterns. Something like this, as an example:

diff --git a/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlParser.java b/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlParser.java
index 516095664..576b33bf9 100644
--- a/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlParser.java
+++ b/rewrite-yaml/src/main/java/org/openrewrite/yaml/YamlParser.java
@@ -41,10 +41,13 @@
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.nio.file.FileSystems;
 import java.nio.file.Path;
+import java.nio.file.PathMatcher;
 import java.util.*;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static java.util.Collections.emptyList;
@@ -53,6 +56,19 @@
 
 public class YamlParser implements org.openrewrite.Parser {
     private static final Pattern VARIABLE_PATTERN = Pattern.compile(":\\s*(@[^\n\r@]+@)");
+    private static final String additionalPatterns = System.getProperty("rewrite.yaml.additionalPatterns", "");
+    private List<PathMatcher> additionalMatchers;
+
+    public YamlParser() {
+        if (!additionalPatterns.isEmpty()) {
+            additionalMatchers = Arrays.stream(additionalPatterns.split(","))
+                    .filter(Objects::nonNull)
+                    .map(pattern -> FileSystems.getDefault().getPathMatcher("glob:" + pattern))
+                    .collect(Collectors.toList());
+        } else {
+            additionalMatchers = new ArrayList<>();
+        }
+    }
 
     @Override
     public Stream<SourceFile> parse(@Language("yml") String... sources) {
@@ -422,7 +438,15 @@ private static int commentAwareIndexOf(char target, String s) {
     @Override
     public boolean accept(Path path) {
         String fileName = path.toString();
-        return fileName.endsWith(".yml") || fileName.endsWith(".yaml");
+        if (fileName.endsWith(".yml") || fileName.endsWith(".yaml")) {
+            return true;
+        }
+
+        if (!additionalMatchers.isEmpty()) {
+            return additionalMatchers.stream().anyMatch(p->p.matches(path));
+        }
+
+        return false;
     }
 
     @Override

Have you considered any alternatives or workarounds?

As a work around, we've had to extend YamlParser and pass our custom filenames at Maven CLI invocation via plainTextMasks. Any recipe which intends to act upon one of our custom filenames must first invoke some internal recipe which parses via our extended parser and exposes the result as Yaml.Documents. This isn't very efficient, and can lead to confusion as users at the company begin using OpenRewrite more.

Additional context

I couldn't find any good examples of extending internals via system properties, but I think the proposed behavior above aligns with how properties are used in the Maven and Gradle plugins.

For example, the Maven plugin has user-facing properties defined like this:

@Parameter(property = "rewrite.plainTextMasks")

And the Gradle plugin has user-facing properties like this:

String maybeProp = System.getProperty("rewrite." + property + "s");

However, there are some settings-specific properties which use a fqn property like these

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/MavenSettings.java:

final String propertyValue = System.getProperty("org.openrewrite.test.readMavenSettingsFromDisk");

rewrite/rewrite-core/src/main/java/org/openrewrite/internal/TreeVisitorAdapterClassLoader.java:

if (System.getProperty("org.openrewrite.adapt.dumpClass") != null) {

rewrite-gradle-plugin/plugin/src/test/kotlin/org/openrewrite/gradle/RewritePluginTest.kt:

val gradleVersion: String? = System.getProperty("org.openrewrite.test.gradleVersion")

I think these are really configuring other tools (Maven, testing, and Gradle respectively), so I don't think the fqn property name would make sense in an exposed yaml option.

Are you interested in contributing this feature to OpenRewrite?

Yes, I can open a PR with some tests.

@jimschubert jimschubert added the enhancement New feature or request label Mar 1, 2024
@timtebeek
Copy link
Contributor

Hi! Thanks for the detailed description of what you're looking for; did you already come across this open pull request?

@timtebeek
Copy link
Contributor

Also; if you'd want to get involved with openrewrite/rewrite-openapi we'd love to have you, or anyone you think might be a good fit.

@jimschubert
Copy link
Author

@timtebeek That other PR looks fantastic. I had searched specifically for issues/PRs related to YAML which is why I missed it. I think my issue can be closed out, or listed as a 'fixes' item on the open PR.

I wasn't aware of rewrite-openapi. I'll take a look at it, although I've admittedly not been active in the OpenAPI space for a few years.

@timtebeek timtebeek linked a pull request Mar 1, 2024 that will close this issue
3 tasks
@timtebeek
Copy link
Contributor

All good, then let's track progress on that other PR and close this suggestion for an alternative implementation. I do appreciate the amount of detail you put into your suggestion here, and providing the context of why it's needed. Hope we'll see more of you in the community. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants