Conversation
TadCordle
requested review from
briandealwis,
chanseokoh,
coollog and
loosebazooka
coollog
left a comment
coollog
left a comment
There was a problem hiding this comment.
Okay for initial versions, but later we should probably consider using a YAML library.
| ImmutableList<Path> paths = | ||
| ImmutableList.of(Paths.get("MANIFEST_PATH_1"), Paths.get("MANIFEST_PATH_2")); | ||
| SkaffoldYamlGenerator generator = new SkaffoldYamlGenerator(paths); | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
nit: might be easier-to-manage/read if this was in a resource file
| /** Automatically generates contents of a skaffold.yaml. */ | ||
| public class SkaffoldYamlGenerator { | ||
|
|
||
| private ImmutableList<Path> manifestPaths; |
| import com.google.common.collect.ImmutableList; | ||
| import java.nio.file.Path; | ||
|
|
||
| /** Automatically generates contents of a skaffold.yaml. */ |
There was a problem hiding this comment.
…from a provided set of Kubernetes manifests. ?
| /** | ||
| * Creates a new {@link SkaffoldYamlGenerator}. | ||
| * | ||
| * @param manifestPaths a non-empty list of paths to kubernetes yamls. Supports glob pattern |
There was a problem hiding this comment.
Hmm, the glob matching seems an odd choice. I'd rather provide a helper to expand a glob to a set of files. Besides the code below doesn't seem to support glob patterns!
There was a problem hiding this comment.
Really it's skaffold that supports glob matching. I guess I should just remove the comment?
There was a problem hiding this comment.
It might make more sense to have the manifestPaths be of type String rather than Path.
|
If we ever plan on using a library to generate this in the future. We should just do that in this library. If that seems overkill for what this code is supposed to do, then this is okay. |
| * | ||
| * @param manifestPaths a non-empty list of paths to kubernetes yamls. Supports glob pattern | ||
| * matching. | ||
| * @param manifestPaths a non-empty list of paths to kubernetes yamls. |
There was a problem hiding this comment.
Oh I just realized Skaffold supports globbing.
| public void testGenerate() throws URISyntaxException, IOException { | ||
| String expected = | ||
| new String( | ||
| Files.readAllBytes(Paths.get(Resources.getResource("yaml/test-skaffold.yaml").toURI())), |
There was a problem hiding this comment.
Could you tie the test resource name to the test? SkaffoldYamlGeneratorTest/generated.yaml?
…ls-for-java into add-skaffold-yaml-generator
| public void testGenerate() throws URISyntaxException, IOException { | ||
| // Read all lines and join with \n to avoid Windows test failing | ||
| List<String> lines = | ||
| Files.readAllLines( |
There was a problem hiding this comment.
Should include charset here.
…ls-for-java into add-skaffold-yaml-generator
|
anybody know if Skaffold has skaffold.yaml generating feature on the roadmap? |
6 participants
Fixes #10