Enable direct runner for github actions for spanner templates - #4252
dhwanilpatel wants to merge 19 commits into
Conversation
Summary of ChangesHello, 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 introduces support for the DirectRunner in Spanner migration templates, enabling local execution of integration tests. The changes include updates to CI/CD tooling to toggle the runner type, adjustments to template logic to accommodate local execution environments, and modifications to existing integration tests to correctly handle resource staging and pathing. Highlights
Ignored Files
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 the 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 counterproductive. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for running integration tests using the DirectRunner instead of the DataflowRunner. It adds a new CLI flag -it-direct-runner to the CI/CD test runner, bypasses Dataflow machine spec validation when DirectRunner is used, and adjusts the integration tests to conditionally configure local truststore paths and skip staging extra files. The reviewer feedback suggests a more robust way to check the directRunnerTest system property in Java by using Boolean.getBoolean("directRunnerTest") instead of checking for null/non-null values, which could fail if the property is explicitly set to "false".
| if (System.getProperty("directRunnerTest") == null) { | ||
| jobParameters.put("extraFilesToStage", truststoreGcsPath); | ||
| } |
There was a problem hiding this comment.
Checking System.getProperty("directRunnerTest") == null is fragile because if the property is explicitly set to "false" (e.g., -DdirectRunnerTest=false), the check will still evaluate as if it is enabled. Using Boolean.getBoolean("directRunnerTest") is much more robust and idiomatic in Java, as it safely parses the property value and returns true only if it is explicitly set to "true".
| if (System.getProperty("directRunnerTest") == null) { | |
| jobParameters.put("extraFilesToStage", truststoreGcsPath); | |
| } | |
| if (!Boolean.getBoolean("directRunnerTest")) { | |
| jobParameters.put("extraFilesToStage", truststoreGcsPath); | |
| } |
| if (System.getProperty("directRunnerTest") != null) { | ||
| truststorePath = cassandraResourceManager.getTrustStoreFile().getAbsolutePath(); | ||
| } |
There was a problem hiding this comment.
Checking System.getProperty("directRunnerTest") != null is fragile because if the property is explicitly set to "false" (e.g., -DdirectRunnerTest=false), the check will still evaluate as if it is enabled. Using Boolean.getBoolean("directRunnerTest") is much more robust and idiomatic in Java, as it safely parses the property value and returns true only if it is explicitly set to "true".
| if (System.getProperty("directRunnerTest") != null) { | |
| truststorePath = cassandraResourceManager.getTrustStoreFile().getAbsolutePath(); | |
| } | |
| if (Boolean.getBoolean("directRunnerTest")) { | |
| truststorePath = cassandraResourceManager.getTrustStoreFile().getAbsolutePath(); | |
| } |
| if (System.getProperty("directRunnerTest") != null) { | ||
| truststoreLocalUrl = "file://" + jdbcResourceManager.getTruststorePath(); | ||
| } |
There was a problem hiding this comment.
Checking System.getProperty("directRunnerTest") != null is fragile because if the property is explicitly set to "false" (e.g., -DdirectRunnerTest=false), the check will still evaluate as if it is enabled. Using Boolean.getBoolean("directRunnerTest") is much more robust and idiomatic in Java, as it safely parses the property value and returns true only if it is explicitly set to "true".
| if (System.getProperty("directRunnerTest") != null) { | |
| truststoreLocalUrl = "file://" + jdbcResourceManager.getTruststorePath(); | |
| } | |
| if (Boolean.getBoolean("directRunnerTest")) { | |
| truststoreLocalUrl = "file://" + jdbcResourceManager.getTruststorePath(); | |
| } |
| if (System.getProperty("directRunnerTest") == null) { | ||
| jobParameters.put("extraFilesToStage", truststoreGcsUrl); | ||
| } |
There was a problem hiding this comment.
Checking System.getProperty("directRunnerTest") == null is fragile because if the property is explicitly set to "false" (e.g., -DdirectRunnerTest=false), the check will still evaluate as if it is enabled. Using Boolean.getBoolean("directRunnerTest") is much more robust and idiomatic in Java, as it safely parses the property value and returns true only if it is explicitly set to "true".
| if (System.getProperty("directRunnerTest") == null) { | |
| jobParameters.put("extraFilesToStage", truststoreGcsUrl); | |
| } | |
| if (!Boolean.getBoolean("directRunnerTest")) { | |
| jobParameters.put("extraFilesToStage", truststoreGcsUrl); | |
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4252 +/- ##
============================================
- Coverage 56.33% 56.33% -0.01%
Complexity 7854 7854
============================================
Files 1155 1155
Lines 73226 73233 +7
Branches 8585 8588 +3
============================================
- Hits 41254 41253 -1
- Misses 29186 29192 +6
- Partials 2786 2788 +2
🚀 New features to boost your workflow:
|
f6c7820 to
097e53c
Compare
202e0b8 to
da3a969
Compare
1 participant
This PR enables directrunner for github actions for Spanner Migration templates.
NOTE: DO NOT REVIEW THE PR AS IT IS IN PROGRESS.