Add matcher and kokoro setup #190
Conversation
Summary of ChangesHello @danjkim, 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 integrates Rust WebAssembly generation capabilities into the CredentialProvider repository. It establishes the foundational logic for matching incoming requests against credential types and sets up a robust Kokoro build pipeline. This pipeline automates the compilation of the Rust WASM module and ensures its proper storage in GCS, streamlining the development and deployment of credential-based services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 Kokoro build configuration and associated scripts for generating a Rust WASM module. The changes are a good step towards automating the build process. However, I've identified several issues that need to be addressed:
- Script Robustness: The
generate_license.shscript lacks error handling (set -e) and suppresses error messages, which could lead to silent failures and difficult debugging. - Path Inconsistencies: There are critical inconsistencies in file paths within the Kokoro configuration (
continuous.cfg) and the build script (kokoro_build.sh). Thestrip_prefixandmvcommand paths do not seem to align with the project structure, which will likely cause the build to fail. - Missing Build Script: The main build logic is in a
./build.shscript which is called bykokoro_build.shbut is not included in this pull request. This makes it impossible to fully review the end-to-end build process.
My detailed comments provide specific suggestions to fix these issues. Please address the critical path issues and improve the script's robustness.
| @@ -0,0 +1,61 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
For robustness, it's a good practice to start bash scripts with set -euo pipefail.
set -e: Exit immediately if a command exits with a non-zero status.set -u: Treat unset variables as an error when substituting.set -o pipefail: The return value of a pipeline is the status of the last command to exit with a non-zero status, or zero if no command exited with a non-zero status.
This will make your script more predictable and prevent it from continuing on errors, for example if cargo vendor fails.
| set -euo pipefail |
| define_artifacts: { | ||
| regex: "**/wasm32-unknown-unknown/release/*.wasm" | ||
| regex: "**/wasm32-unknown-unknown/release/Cargo.lock" | ||
| strip_prefix: "github/wasm/CredentialProvider/wasm/target/wasm32-unknown-unknown/release" |
There was a problem hiding this comment.
The strip_prefix path appears to be incorrect. Given the project structure with the wasm project likely at CredentialProvider/wasm/credential_transfer, the build artifacts would be generated inside its target directory. The prefix should probably include credential_transfer to correctly locate the artifacts.
| strip_prefix: "github/wasm/CredentialProvider/wasm/target/wasm32-unknown-unknown/release" | |
| strip_prefix: "github/wasm/CredentialProvider/wasm/credential_transfer/target/wasm32-unknown-unknown/release" |
| ./build.sh | ||
|
|
||
| # Move Cargo.lock to artifacts directory for SBOM generation | ||
| mv CredentialProvider/Cargo.lock "${KOKORO_ARTIFACTS_DIR}/github/wasm/CredentialProvider/target/wasm32-unknown-unknown/release/" |
There was a problem hiding this comment.
The source and destination paths for mv seem inconsistent with both the likely project structure and the strip_prefix in continuous.cfg.
- The source
CredentialProvider/Cargo.locksuggests the Rust project is inCredentialProvider, but the new files in this PR are inCredentialProvider/wasm/credential_transfer. - The destination path
.../CredentialProvider/target/...is inconsistent with thestrip_prefixincontinuous.cfgwhich is.../CredentialProvider/wasm/target/....
Please verify the project structure and update the paths accordingly. If the project is at CredentialProvider/wasm/credential_transfer, the command should be something like:
mv CredentialProvider/wasm/credential_transfer/Cargo.lock CredentialProvider/wasm/credential_transfer/target/wasm32-unknown-unknown/release/
|
|
||
| # 2. Vendor dependencies (download source code to local folder) | ||
| echo "📦 Vendorizing dependencies..." | ||
| cargo vendor "$VENDOR_DIR" > /dev/null 2>&1 |
There was a problem hiding this comment.
Redirecting both stdout and stderr to /dev/null will hide any errors from cargo vendor. If the command fails, the script will either exit silently (with set -e) or continue with an empty/incomplete vendor directory, making debugging difficult. It's better to remove the redirection to see potential errors.
| cargo vendor "$VENDOR_DIR" > /dev/null 2>&1 | |
| cargo vendor "$VENDOR_DIR" |
| if [ "$license_found" = false ]; then | ||
| echo "WARNING: No license file found in source." >> "$OUTPUT_FILE" | ||
| fi |
There was a problem hiding this comment.
Warnings about missing license files are important for visibility. Writing them only to the output file might cause them to be missed. It's a good practice to also print such warnings to standard error, so they are visible in the build logs. Also, including the package name in the warning message will make it more informative.
| if [ "$license_found" = false ]; then | |
| echo "WARNING: No license file found in source." >> "$OUTPUT_FILE" | |
| fi | |
| if [ "$license_found" = false ]; then | |
| echo "WARNING: No license file found for '$package_name' in source." >&2 | |
| echo "WARNING: No license file found in source." >> "$OUTPUT_FILE" | |
| fi |
Adding rust wasm generation logic to the CredentialProvider repo. The logic will match the incoming request with the registered export entries by looking at credential types. Kokoro build system will trigger the build of the wasm repo and store the newly generated wasm into GCS compliant with BCID_L1. Add matcher source code for credential exchange Add source code for credential exchange matcher. Create a build script that runs the Makefile. fixing github suggestions
1 participant
Adding rust wasm generation logic to the CredentialProvider repo. The logic will match the incoming request with the registered export entries by looking at credential types. Kokoro build system will trigger the build of the wasm repo and store the newly generated wasm into GCS compliant with BCID_L1.