-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add yaml blueprints publishing to GCS #3056
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
base: main
Are you sure you want to change the base?
Add yaml blueprints publishing to GCS #3056
Conversation
Summary of ChangesHello @derrickaw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (50.98%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3056 +/- ##
============================================
+ Coverage 50.79% 50.86% +0.06%
- Complexity 5116 5125 +9
============================================
Files 976 976
Lines 59994 60031 +37
Branches 6555 6559 +4
============================================
+ Hits 30476 30533 +57
+ Misses 27380 27348 -32
- Partials 2138 2150 +12
🚀 New features to boost your workflow:
|
|
/gemini review |
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.
Code Review
This pull request adds the capability to publish YAML blueprints to Google Cloud Storage during the release process. The changes are logical and well-contained, introducing a new Mojo parameter to control this feature and the corresponding implementation to handle the file uploads. New tests are also included to validate the happy path of this new functionality. My review focuses on improving the robustness, efficiency, and configurability of the new upload logic. I've identified a few areas for improvement, including a logic error that prevents graceful skipping, a potential resource leak, inefficient file handling, and the use of hardcoded paths. I've also provided suggestions to enhance the test coverage.
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Show resolved
Hide resolved
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Outdated
Show resolved
Hide resolved
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Show resolved
Hide resolved
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request enhances the TemplatesReleaseMojo Maven plugin by adding functionality to publish YAML blueprints to Google Cloud Storage. The changes include introducing new Maven parameters (publishYamlBlueprints, yamlBlueprintsPath, yamlBlueprintsGCSBucket), adding google-cloud-storage and mockito-inline dependencies, and implementing logic to read YAML files from a local path and upload them to a specified GCS bucket. The existing template staging process was refactored to integrate with this new feature. A new test file, TemplatesReleaseMojoTest, was added to verify the YAML blueprint upload mechanism, utilizing mocked GCS services. A review comment suggests improving the test by using java.nio.charset.StandardCharsets.UTF_8 instead of the string literal "UTF-8" when converting bytes to a string, to avoid UnsupportedEncodingException and increase code robustness.
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request introduces the capability to publish YAML blueprints to Google Cloud Storage as part of the release process. The changes include adding the necessary GCS dependency, implementing the upload logic in TemplatesReleaseMojo, and adding comprehensive unit tests for the new functionality. The implementation is solid, but I've identified a potential resource leak with the Storage client object which should be addressed. I've provided a suggestion to fix this using a try-with-resources statement, which also simplifies the code structure.
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request adds a new capability to the templates-maven-plugin to publish YAML blueprints to Google Cloud Storage during the release process. The changes include adding the google-cloud-storage dependency, introducing new parameters to control the publishing behavior, and implementing the upload logic in TemplatesReleaseMojo. Commendably, comprehensive unit tests have been added in TemplatesReleaseMojoTest to validate the new functionality, covering success cases, and scenarios where uploads should be skipped.
The implementation is solid, but I have a couple of suggestions to enhance robustness and code clarity:
- In
TemplatesReleaseMojo.java, the file filtering logic for YAML blueprints can be made more robust to correctly handle directories and be platform-independent. - In
TemplatesReleaseMojoTest.java, the test code can be made more idiomatic by usingAtomicReferencefor capturing state within a lambda expression.
Overall, this is a great addition. Please see my detailed comments.
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Outdated
Show resolved
Hide resolved
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request adds the functionality to publish YAML blueprints to Google Cloud Storage as part of the release process. The changes include adding the google-cloud-storage dependency, introducing new Maven parameters to control the publishing, and implementing the upload logic in TemplatesReleaseMojo. A comprehensive set of unit tests has been added in TemplatesReleaseMojoTest to verify the new functionality, including success and failure scenarios. The implementation looks solid. I have one minor suggestion in the test code to improve its robustness across different environments.
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request adds a feature to publish YAML blueprints to GCS as part of the templates-maven-plugin. The changes include adding necessary dependencies, new configuration parameters, and the implementation logic for uploading files. New tests are also added to cover the new functionality. The implementation looks solid and is well-tested. I've added a couple of minor suggestions to improve code style by using imports instead of fully qualified class names for better readability.
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Outdated
Show resolved
Hide resolved
...en-plugin/src/test/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojoTest.java
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request adds functionality to publish YAML blueprints to Google Cloud Storage as part of the release process. The changes include adding necessary dependencies, implementing the publishing logic in TemplatesReleaseMojo, and adding comprehensive unit tests for the new feature. The implementation is solid, and the tests cover success and failure cases well. I have one suggestion to improve the exception handling in the new publishing logic for better maintainability.
...-maven-plugin/src/main/java/com/google/cloud/teleport/plugin/maven/TemplatesReleaseMojo.java
Show resolved
Hide resolved
chamikaramj
left a comment
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.
Thanks!
|
|
||
| @Parameter( | ||
| defaultValue = "yaml-blueprints", | ||
| property = "yamlBlueprintsGCSBucket", |
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.
Are we always uploading to the top level of the GCS bucket ? May be this should be a GCS path ?
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 believe we are. We have the standard: gs://bucketName/stagePrefix/yamlBlueprintsGCSBucket/fileName
So you would rather give a path at that point, so something like this that could be placed anymore under stagePrefix?
gs://bucketName/stagePrefix/yamlBlueprintsGCSBucketPath/fileName
Thanks
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.
Went ahead with the GCSPath name, which is what I think you were alluding to. Thanks.
| } | ||
| } | ||
| } else { | ||
| LOG.warn("Not found templates to release in this module."); |
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.
Did not find ...
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.
done, thanks
| if (definition.isFlex()) { | ||
| generator.saveImageSpec(definition, imageSpec, targetDirectory); | ||
| if (publishYamlBlueprints) { | ||
| LOG.info("Trying to upload YAML blueprints to bucket '{}'...", bucketNameOnly(bucketName)); |
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.
Job builder blueprints
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.
done, thanks
| LOG.info("Trying to upload YAML blueprints to bucket '{}'...", bucketNameOnly(bucketName)); | ||
| Path yamlPath = Paths.get(project.getBasedir().getAbsolutePath(), yamlBlueprintsPath); | ||
| if (!Files.exists(yamlPath) || !Files.isDirectory(yamlPath)) { | ||
| LOG.warn("YAML blueprints directory not found, skipping upload: {}", yamlPath); |
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.
Just fail the script if no files are found ?
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.
Valid point, done, thanks
| path -> | ||
| Files.isRegularFile(path) | ||
| && path.getFileName().toString().endsWith(".yaml")) | ||
| .forEach( |
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 to clarify, the YAML files under "yaml/src/main/yaml" uploaded to GCS here are directly consumable by Job builder without any transformations ?
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 intention is to make the yaml format the standard, which may require some modifications on JB, but not the yaml files themselves.
1a00610 to
a776aa9
Compare
|
Hi @damccorm, if you prefer to wait on Cham's final approval that is cool. Thanks. |
|
Yeah, I'll let Cham finish the review here since it is already in progress. Thanks |
Uh oh!
There was an error while loading. Please reload this page.