feat(cli): Makefile and build scripts#3208
feat(cli): Makefile and build scripts#3208alkalescent merged 1 commit intoDSPX-2655-migrate-otdfctlfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
alkalescent
changed the title
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 refines the build infrastructure by integrating the 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. Makefiles hum and sing, Scripts now neatly placed, they bring, Builds are clean and fast. Footnotes
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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 a new otdfctl command-line tool, integrating it into the main build system via Makefile updates for building, cleaning, and managing dependencies. It also adds new shell scripts, zip-builds.sh and verify-checksums.sh, within the otdfctl/scripts directory to automate binary packaging (zipping for Windows, tar-gzipping for others) and checksum verification for releases. The otdfctl/Makefile was updated to reference these new local scripts. The review feedback suggests improvements for both new scripts: zip-builds.sh should handle no-match globs, standardize archive structure by placing binaries at the root of the archive, and generate checksums in a standard format. verify-checksums.sh should be updated to parse the standard checksum filename format and exit with a non-zero status code if any checksum verification fails to ensure proper CI/CD integration.
| # Iterate over each line in the checksum file | ||
| while read -r line; do | ||
| # Extract the expected checksum and filename from each line | ||
| read -ra ADDR <<< "$line" # Read the line into an array | ||
| expectedChecksum="${ADDR[0]}" | ||
| fileName="${ADDR[2]}" | ||
|
|
||
| # Calculate the actual checksum of the file | ||
| actualChecksum=$(shasum -a 256 "$outputDir/$fileName" | awk '{print $1}') | ||
|
|
||
| # Compare the expected checksum with the actual checksum | ||
| if [ "$expectedChecksum" == "$actualChecksum" ]; then | ||
| echo "SUCCESS: Checksum for $fileName is valid." | ||
| else | ||
| echo "ERROR: Checksum for $fileName does not match." | ||
| fi | ||
| done < "$checksumFile" |
There was a problem hiding this comment.
This script can be improved for robustness and to align with the suggested changes in zip-builds.sh:
- Handle standard checksum format: The verification logic should be updated to parse the standard
checksum filenameformat. Usingreadwith two variables is cleaner than creating an array. - Report failure correctly: The script should exit with a non-zero status code if any checksum verification fails. This is crucial for CI/CD pipelines to detect build failures.
| # Iterate over each line in the checksum file | |
| while read -r line; do | |
| # Extract the expected checksum and filename from each line | |
| read -ra ADDR <<< "$line" # Read the line into an array | |
| expectedChecksum="${ADDR[0]}" | |
| fileName="${ADDR[2]}" | |
| # Calculate the actual checksum of the file | |
| actualChecksum=$(shasum -a 256 "$outputDir/$fileName" | awk '{print $1}') | |
| # Compare the expected checksum with the actual checksum | |
| if [ "$expectedChecksum" == "$actualChecksum" ]; then | |
| echo "SUCCESS: Checksum for $fileName is valid." | |
| else | |
| echo "ERROR: Checksum for $fileName does not match." | |
| fi | |
| done < "$checksumFile" | |
| # Iterate over each line in the checksum file | |
| had_error=0 | |
| while read -r expectedChecksum fileName; do | |
| # Skip empty lines | |
| [ -z "$expectedChecksum" ] && continue | |
| # Calculate the actual checksum of the file | |
| actualChecksum=$(shasum -a 256 "$outputDir/$fileName" | awk '{print $1}') | |
| # Compare the expected checksum with the actual checksum | |
| if [ "$expectedChecksum" == "$actualChecksum" ]; then | |
| echo "SUCCESS: Checksum for $fileName is valid." | |
| else | |
| echo "ERROR: Checksum for $fileName does not match." | |
| had_error=1 | |
| fi | |
| done < "$checksumFile" | |
| exit $had_error |
| for binary_file in "$binary_dir"/*; do | ||
| compressed="" | ||
| if [[ $binary_file == *.exe ]]; then | ||
| # If the file is a Windows binary, zip it | ||
| filename=$(basename "$binary_file") | ||
| compressed="${filename%.exe}.zip" | ||
| zip -j "$output_dir/$compressed" "$binary_file" | ||
| else | ||
| # For other binaries, tar and gzip them | ||
| filename=$(basename "$binary_file") | ||
| compressed="${filename}.tar.gz" | ||
| tar -czf "$output_dir/$compressed" "$binary_file" | ||
| fi | ||
|
|
||
| # Append checksums to the file | ||
| echo "$(cat "$output_dir/$compressed" | shasum -a 256) $compressed" >> $checksums_file | ||
| done |
There was a problem hiding this comment.
This script can be improved for robustness and to follow standard practices:
- Handle no-match globs: The loop
for binary_file in "$binary_dir"/*will execute with a literal*if no files are found. Add a check to skip non-existent files. - Standardize archive structure: The
tarcommand currently includes thetarget/directory in the archive. Use the-Coption to place the binary at the root of the archive, matching the behavior of thezipcommand for Windows binaries. - Standardize checksum format: The current method of generating checksums is unconventional. It's better to generate a standard checksum file that can be verified with tools like
shasum -c. This also simplifies the verification script.
Here is a suggested replacement for the loop that addresses these points. Note that this change will require a corresponding update to verify-checksums.sh.
| for binary_file in "$binary_dir"/*; do | |
| compressed="" | |
| if [[ $binary_file == *.exe ]]; then | |
| # If the file is a Windows binary, zip it | |
| filename=$(basename "$binary_file") | |
| compressed="${filename%.exe}.zip" | |
| zip -j "$output_dir/$compressed" "$binary_file" | |
| else | |
| # For other binaries, tar and gzip them | |
| filename=$(basename "$binary_file") | |
| compressed="${filename}.tar.gz" | |
| tar -czf "$output_dir/$compressed" "$binary_file" | |
| fi | |
| # Append checksums to the file | |
| echo "$(cat "$output_dir/$compressed" | shasum -a 256) $compressed" >> $checksums_file | |
| done | |
| for binary_file in "$binary_dir"/*; do | |
| [ -f "$binary_file" ] || continue # Handle case where no files match | |
| filename=$(basename "$binary_file") | |
| compressed="" | |
| if [[ $binary_file == *.exe ]]; then | |
| # If the file is a Windows binary, zip it | |
| compressed="${filename%.exe}.zip" | |
| zip -j "$output_dir/$compressed" "$binary_file" | |
| else | |
| # For other binaries, tar and gzip them without parent path | |
| compressed="${filename}.tar.gz" | |
| tar -czf "$output_dir/$compressed" -C "$(dirname "$binary_file")" "$filename" | |
| fi | |
| # Append checksums to the file in standard format | |
| (cd "$output_dir" && shasum -a 256 "$compressed") >> "$checksums_file" | |
| done |
alkalescent
force-pushed
the
DSPX-2655-migrate-otdfctl
branch
from
01cff81 to
d66bd5b
Compare
alkalescent
force-pushed
the
DSPX-2655-migrate-otdfctl
branch
2 times, most recently
from
fc2fac5 to
24366f4
Compare
alkalescent
force-pushed
the
DSPX-2657-otdfctl-build
branch
from
146adcf to
a55588b
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
### Proposed Changes * This PR updates the respective Makefiles for the monorepo and otdfctl as well as restoring the build scripts from the original otdfctl repo. ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
* This PR updates the respective Makefiles for the monorepo and otdfctl as well as restoring the build scripts from the original otdfctl repo. - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation
### Proposed Changes * Add otdfctl component to platform release-please configuration for independent versioned releases * Tags follow the monorepo per-component pattern: `otdfctl/v0.30.0` * Register `otdfctl/pkg/config/config.go` as extra-file so release-please bumps the `Version` constant (already has `// x-release-please-version` marker) * Create release workflow that triggers on `otdfctl/v*` tags, builds 8 cross-platform binaries (darwin amd64/arm64, linux amd64/arm/arm64, windows amd64/arm/arm64), and uploads artifacts to the GitHub release #### Files added/modified | File | Change | |------|--------| | `release-please-config.main.json` | Add `otdfctl` package entry with `extra-files` | | `release-please-manifest.json` | Add `"otdfctl": "0.30.0"` version tracking | | `release-please-config.otdfctl.json` | **New** — component config for `release/otdfctl/vX.Y` branches | | `release-otdfctl.yaml` | **New** — build and upload workflow on release publish | #### PR Stack (DSPX-2654) 1. #3205 — Subtree merge + module path rewrite (DSPX-2655, DSPX-2656) 2. #3208 — Makefile and build scripts (DSPX-2657) 3. #3221 — CI workflows (DSPX-2658) 4. #3236 — e2e tests and lint fixes (DSPX-2659) 5. **This PR** — Release pipeline (DSPX-2660) ### Checklist - [ ] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions - Verify JSON configs are valid: `cat .github/release-please/release-please-config.main.json | jq .packages.otdfctl` - Verify manifest version: `cat .github/release-please/release-please-manifest.json | jq .otdfctl` - Verify `reusable_release-please.yaml` config lookup: branch `release/otdfctl/v0.30` → sanitized name `otdfctl` → resolves to `release-please-config.otdfctl.json` - Full release flow testable after merge by creating a manual release with tag `otdfctl/v0.30.0` --------- Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
2 participants
Proposed Changes
Checklist
Testing Instructions