-
Notifications
You must be signed in to change notification settings - Fork 8
feat(main): add "INSTALL.md" file #726
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?
Conversation
Summary of ChangesHello @b-long, 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 a new 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
|
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 INSTALL.md file with detailed installation instructions for otdfctl. The guide is well-structured for different operating systems. My review includes a critical fix for the macOS/Linux installation scripts, which would likely fail as written. I've also suggested improvements for maintainability by reducing hardcoded version strings, and enhancing the user-friendliness of the Windows installation steps by avoiding the need for administrator privileges. Finally, I've pointed out a more direct link for the documentation.
| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-arm64.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-darwin-arm64.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-darwin-arm64.tar.gz |
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.
This installation script has two issues:
- Critical Bug: The
mvcommand on line 42 will likely fail because it assumes the binary is in atarget/subdirectory. Release archives usually don't contain build-time directory structures. This will break the installation. - Maintainability: The version
0.28.0is hardcoded. This makes updating the documentation for new releases tedious and error-prone.
The suggested change below fixes both issues by removing target/ and using a VERSION variable. This makes the script more robust and easier to maintain. Similar changes should be applied to all other script blocks for macOS and Linux.
# Set the version you want to install
VERSION="0.28.0"
TARBALL="otdfctl-${VERSION}-darwin-arm64.tar.gz"
BINARY="otdfctl-${VERSION}-darwin-arm64"
# Download the release archive
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${TARBALL}"
# Extract the file
tar -xzf "${TARBALL}"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv "${BINARY}" /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${TARBALL}"
INSTALL.md
Outdated
| 2. **Extract the ZIP file:** | ||
| - Right-click the downloaded file | ||
| - Select "Extract All..." | ||
| - Choose a location (e.g., `C:\Program Files\otdfctl`) |
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.
Suggesting C:\Program Files as the extraction location requires administrator privileges. For a better user experience, especially for users without admin rights, consider suggesting a user-specific directory instead, which doesn't require elevation.
| - Choose a location (e.g., `C:\Program Files\otdfctl`) | |
| - Choose a location (e.g., `%USERPROFILE%\otdfctl`) |
X-Test Results✅ js-v0.4.34 |
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.
Pull request overview
This PR adds comprehensive installation documentation for otdfctl by introducing a new INSTALL.md file with step-by-step instructions for macOS, Linux, and Windows users. The documentation includes platform-specific installation commands, troubleshooting guidance, and verification steps.
Key Changes:
- New
INSTALL.mdfile with detailed installation instructions for all major platforms - Updated release-please configuration files to include version tracking for the new documentation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| INSTALL.md | New installation guide with platform-specific instructions, architecture detection guidance, and troubleshooting tips |
| .github/release-please/release-please-config.release_branches.json | Added INSTALL.md to version-tracked files for release branch |
| .github/release-please/release-please-config.main.json | Added INSTALL.md to version-tracked files for main branch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **For Apple Silicon Macs (M1/M2/M3):** | ||
| ```bash | ||
| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-arm64.tar.gz |
Copilot
AI
Dec 19, 2025
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 version number 0.28.0 is hardcoded throughout the installation instructions (appearing in lines 33, 36, 42, 48, 54, 57, 63, 69, 94, 97, 103, 109, 115, 118, 124, 130, 136, 139, 145, 151, 165, 166, 194). This creates a maintenance burden when updating versions. Consider adding a note at the beginning of the document instructing users to replace 0.28.0 with the latest version number, or create a variable placeholder that can be easily updated in one place.
| - Press `Windows key` and search for "Environment Variables" | ||
| - Click "Edit environment variables for your account" | ||
| - In the "User variables" section at the top, find and select "Path", then click "Edit...". (If "Path" doesn't exist, click "New..." to create it). | ||
| - Click "New" and add the folder path where you extracted `otdfctl.exe` (e.g., `%USERPROFILE%\otdfctl`) |
Copilot
AI
Dec 19, 2025
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 example path %USERPROFILE%\otdfctl does not match the suggested extraction location from step 2 (C:\Program Files\otdfctl). This inconsistency may confuse users. Either use the same path in both places or clarify that users should use whichever path they chose during extraction.
| - Click "New" and add the folder path where you extracted `otdfctl.exe` (e.g., `%USERPROFILE%\otdfctl`) | |
| - Click "New" and add the folder path where you extracted `otdfctl.exe` (e.g., `C:\Program Files\otdfctl`) |
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 an INSTALL.md file with installation instructions for otdfctl. The instructions are clear and well-structured for different operating systems. However, I've identified a few critical issues, primarily related to hardcoded version numbers and incorrect shell commands, which could cause the installation to fail and create a maintenance burden. My review includes suggestions to use variables for versioning and to correct the file paths in the installation scripts. Addressing these points will make the documentation more robust and user-friendly.
| **For Apple Silicon Macs (M1/M2/M3):** | ||
| ```bash | ||
| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-arm64.tar.gz |
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 version v0.28.0 is hardcoded in this URL. This version is hardcoded throughout the file in other URLs, filenames, and example outputs. This creates a significant maintenance burden, as the file will need to be manually updated in multiple places for every new release.
To improve maintainability, consider using a variable for the version number at the beginning of each script block. This would allow for a single point of update per script and reduce the chance of errors.
Example:
VERSION="0.28.0"
curl -LO https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/otdfctl-${VERSION}-darwin-arm64.tar.gz
# ... and use ${VERSION} in subsequent commands| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl |
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 path target/otdfctl-0.28.0-darwin-arm64 in the mv command is likely incorrect and will cause the installation to fail. The tar command typically extracts files to the current directory, and it's unlikely to create a target/ subdirectory. Furthermore, the executable file inside the archive is probably named otdfctl, not the full archive name.
This same issue exists on lines 63, 103, 124, and 145.
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl | |
| sudo mv otdfctl /usr/local/bin/otdfctl |
|
|
||
| You should see output like: | ||
| ``` | ||
| otdfctl version 0.28.0 |
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.
Similar to other parts of the file, the version number here is hardcoded. This might confuse users if they install a different version than the one mentioned. It would be better to show a generic output to avoid confusion and make the documentation more version-agnostic.
| otdfctl version 0.28.0 | |
| otdfctl version <VERSION> |
29f76fb to
5ff1049
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **For Apple Silicon Macs (M1/M2/M3):** | ||
| ```bash | ||
| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-arm64.tar.gz |
Copilot
AI
Dec 19, 2025
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 version number 0.28.0 is hardcoded throughout the installation guide. This creates maintenance burden as every version bump will require updating multiple locations. Consider adding a note at the top of the document instructing users to replace version numbers with the latest release, or use a variable/placeholder pattern like VERSION=0.28.0 that users can set once.
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | ||
| - The executable will be in this folder (you may need to navigate into a subfolder) |
Copilot
AI
Dec 19, 2025
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 instruction to 'create a folder' during extraction is unclear. Users should extract the ZIP file first, then move the executable to %USERPROFILE%\otdfctl. The current wording suggests creating the folder before extraction, which may confuse users about where to extract the files initially.
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | |
| - The executable will be in this folder (you may need to navigate into a subfolder) | |
| - Choose any temporary location (for example, your `Downloads` folder) and complete the extraction | |
| - After extraction, create the folder `%USERPROFILE%\otdfctl` if it doesn't already exist | |
| - Move `otdfctl.exe` from the extracted folder into `%USERPROFILE%\otdfctl` |
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 an INSTALL.md file with installation instructions for otdfctl and updates the release configuration. The new installation guide is a great addition for new users. However, I've found several critical issues in the installation scripts for macOS and Linux that will cause the installation to fail. The scripts use incorrect paths for moving the binary. Additionally, all instructions (including for Windows) use hardcoded version numbers, which will make this document difficult to maintain and quickly outdated. My review includes suggestions to fix the broken commands and refactor the scripts and links to be more maintainable.
| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-arm64.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-darwin-arm64.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-darwin-arm64.tar.gz |
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 installation script has two issues:
- Incorrect Path: The
mvcommand on line 42 uses an incorrect pathtarget/otdfctl-0.28.0-darwin-arm64. Thetar.gzarchive contains a single executable namedotdfctlat the root, andtardoes not create atargetdirectory. This will cause the installation to fail. - Hardcoded Values: The version number and architecture are hardcoded in multiple places. This makes the script difficult to maintain and prone to errors when updating.
I suggest refactoring the script to use variables. This makes it more readable, easier to update, and fixes the incorrect path.
# Download the latest version
VERSION="0.28.0"
OS="darwin"
ARCH="arm64"
FILENAME="otdfctl-${VERSION}-${OS}-${ARCH}"
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${FILENAME}.tar.gz"
# Extract the file
tar -xzf "${FILENAME}.tar.gz"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv otdfctl /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${FILENAME}.tar.gz"| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-darwin-amd64.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-darwin-amd64.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-amd64 /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-darwin-amd64.tar.gz |
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.
Similar to the Apple Silicon script, this script has an incorrect mv path and hardcoded values. The mv command on line 63 will fail because the path target/otdfctl-0.28.0-darwin-amd64 is incorrect. I recommend refactoring to use variables for maintainability and correctness.
# Download the latest version
VERSION="0.28.0"
OS="darwin"
ARCH="amd64"
FILENAME="otdfctl-${VERSION}-${OS}-${ARCH}"
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${FILENAME}.tar.gz"
# Extract the file
tar -xzf "${FILENAME}.tar.gz"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv otdfctl /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${FILENAME}.tar.gz"| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-linux-amd64.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-linux-amd64.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-amd64 /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-linux-amd64.tar.gz |
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.
This script also contains an incorrect mv path (line 103) and hardcoded values. The path target/otdfctl-0.28.0-linux-amd64 is incorrect and will cause the installation to fail. Please refactor it for correctness and easier maintenance.
# Download the latest version
VERSION="0.28.0"
OS="linux"
ARCH="amd64"
FILENAME="otdfctl-${VERSION}-${OS}-${ARCH}"
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${FILENAME}.tar.gz"
# Extract the file
tar -xzf "${FILENAME}.tar.gz"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv otdfctl /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${FILENAME}.tar.gz"| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-linux-arm64.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-linux-arm64.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-arm64 /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-linux-arm64.tar.gz |
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 mv command on line 124 has an incorrect path, and the script uses hardcoded values. The path target/otdfctl-0.28.0-linux-arm64 does not exist after extraction, which will break the installation. I recommend refactoring to use variables for correctness and maintainability.
# Download the latest version
VERSION="0.28.0"
OS="linux"
ARCH="arm64"
FILENAME="otdfctl-${VERSION}-${OS}-${ARCH}"
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${FILENAME}.tar.gz"
# Extract the file
tar -xzf "${FILENAME}.tar.gz"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv otdfctl /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${FILENAME}.tar.gz"| # Download the latest version | ||
| curl -LO https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-linux-arm.tar.gz | ||
|
|
||
| # Extract the file | ||
| tar -xzf otdfctl-0.28.0-linux-arm.tar.gz | ||
|
|
||
| # Create the directory if it doesn't exist | ||
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-arm /usr/local/bin/otdfctl | ||
|
|
||
| # Make it executable | ||
| sudo chmod +x /usr/local/bin/otdfctl | ||
|
|
||
| # Clean up the downloaded file | ||
| rm otdfctl-0.28.0-linux-arm.tar.gz |
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 mv command on line 145 is incorrect as the path target/otdfctl-0.28.0-linux-arm does not exist after extraction. Additionally, the script uses hardcoded values, making it faulty and hard to maintain. Please refactor it for correctness.
# Download the latest version
VERSION="0.28.0"
OS="linux"
ARCH="arm"
FILENAME="otdfctl-${VERSION}-${OS}-${ARCH}"
curl -LO "https://github.com/opentdf/otdfctl/releases/download/v${VERSION}/${FILENAME}.tar.gz"
# Extract the file
tar -xzf "${FILENAME}.tar.gz"
# Create the directory if it doesn't exist
sudo mkdir -p /usr/local/bin
# Move it to a folder in your PATH
sudo mv otdfctl /usr/local/bin/otdfctl
# Make it executable
sudo chmod +x /usr/local/bin/otdfctl
# Clean up the downloaded file
rm "${FILENAME}.tar.gz"| 1. **Download the file** for your system: | ||
| - For most PCs: [otdfctl-0.28.0-windows-amd64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-amd64.zip) | ||
| - For ARM devices: [otdfctl-0.28.0-windows-arm64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-arm64.zip) | ||
|
|
||
| 2. **Extract the ZIP file:** | ||
| - Right-click the downloaded file | ||
| - Select "Extract All..." | ||
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | ||
| - The executable will be in this folder (you may need to navigate into a subfolder) |
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.
There are a couple of areas for improvement in the Windows installation instructions:
- Hardcoded Links: The download links on lines 165-166 point to a specific version (
v0.28.0). These will become outdated with the next release. It's better to direct users to the "Latest Release" page where they can find the correct assets. - Incorrect Extraction Path: The note on line 172 mentions that the user might need to navigate into a subfolder. However, the
.ziparchives for Windows contain theotdfctl.exeexecutable at the root, so this note is misleading and should be removed.
| 1. **Download the file** for your system: | |
| - For most PCs: [otdfctl-0.28.0-windows-amd64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-amd64.zip) | |
| - For ARM devices: [otdfctl-0.28.0-windows-arm64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-arm64.zip) | |
| 2. **Extract the ZIP file:** | |
| - Right-click the downloaded file | |
| - Select "Extract All..." | |
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | |
| - The executable will be in this folder (you may need to navigate into a subfolder) | |
| 1. **Download the file** for your system from the [latest release page](https://github.com/opentdf/otdfctl/releases/latest). Look for the `.zip` file that matches your system architecture (e.g., `otdfctl-x.y.z-windows-amd64.zip`). | |
| 2. **Extract the ZIP file:** | |
| - Right-click the downloaded file | |
| - Select "Extract All..." | |
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | |
| - The executable will be in this folder. |
58ae91c to
17bdc07
Compare
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
Co-authored-by: Copilot <[email protected]>
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 an INSTALL.md file with detailed installation instructions for otdfctl on macOS, Linux, and Windows. It also configures release-please to automatically update version numbers in this new file, which is a great approach. My review focuses on improving the clarity and correctness of the installation steps. I've identified that the installation commands for macOS and Linux will likely fail due to an incorrect path and have provided suggestions to fix them. I've also provided feedback on improving the Windows installation experience, as requested in the pull request description. Overall, this is a valuable addition to the project's documentation.
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl |
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 path target/ in this command is likely incorrect. Release archives typically don't contain the target directory from the build environment, so this command will probably fail. Please verify the contents of the .tar.gz file and adjust the path. Assuming the executable is at the root of the archive, the command should be as suggested.
| sudo mv target/otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl | |
| sudo mv otdfctl-0.28.0-darwin-arm64 /usr/local/bin/otdfctl |
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-darwin-amd64 /usr/local/bin/otdfctl |
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.
As with the arm64 instruction, the target/ path here is likely incorrect and will cause the installation to fail. Please verify the archive contents and update the path.
| sudo mv target/otdfctl-0.28.0-darwin-amd64 /usr/local/bin/otdfctl | |
| sudo mv otdfctl-0.28.0-darwin-amd64 /usr/local/bin/otdfctl |
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-amd64 /usr/local/bin/otdfctl |
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 target/ path here is likely incorrect, as release archives usually don't include build directories. This will cause the installation to fail. Please verify the archive contents and update the path.
| sudo mv target/otdfctl-0.28.0-linux-amd64 /usr/local/bin/otdfctl | |
| sudo mv otdfctl-0.28.0-linux-amd64 /usr/local/bin/otdfctl |
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-arm64 /usr/local/bin/otdfctl |
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.
As with the other installation commands, the target/ path here is likely incorrect and will cause the installation to fail. Please verify the archive contents and update the path.
| sudo mv target/otdfctl-0.28.0-linux-arm64 /usr/local/bin/otdfctl | |
| sudo mv otdfctl-0.28.0-linux-arm64 /usr/local/bin/otdfctl |
| sudo mkdir -p /usr/local/bin | ||
|
|
||
| # Move it to a folder in your PATH | ||
| sudo mv target/otdfctl-0.28.0-linux-arm /usr/local/bin/otdfctl |
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.
As with the other installation commands, the target/ path here is likely incorrect and will cause the installation to fail. Please verify the archive contents and update the path.
| sudo mv target/otdfctl-0.28.0-linux-arm /usr/local/bin/otdfctl | |
| sudo mv otdfctl-0.28.0-linux-arm /usr/local/bin/otdfctl |
| <details> | ||
| <summary><h3>For Windows Users</h3></summary> | ||
|
|
||
| #### Step 1: Download the Right Version | ||
|
|
||
| Most Windows computers use `amd64`. ARM versions are for Surface Pro X or other ARM-based Windows devices. | ||
|
|
||
| #### Step 2: Download and Install | ||
|
|
||
| 1. **Download the file** for your system: | ||
| - For most PCs: [otdfctl-0.28.0-windows-amd64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-amd64.zip) | ||
| - For ARM devices: [otdfctl-0.28.0-windows-arm64.zip](https://github.com/opentdf/otdfctl/releases/download/v0.28.0/otdfctl-0.28.0-windows-arm64.zip) | ||
|
|
||
| 2. **Extract the ZIP file:** | ||
| - Right-click the downloaded file | ||
| - Select "Extract All..." | ||
| - Choose a location and create a folder: `%USERPROFILE%\otdfctl` | ||
| - The executable will be in this folder (you may need to navigate into a subfolder) | ||
|
|
||
| 3. **Add to PATH:** | ||
| - Press `Windows key` and search for "Environment Variables" | ||
| - Click "Edit environment variables for your account" | ||
| - In the "User variables" section at the top, find and select "Path", then click "Edit...". (If "Path" doesn't exist, click "New..." to create it). | ||
| - Click "New" and add the folder path where you extracted `otdfctl.exe` (e.g., `%USERPROFILE%\otdfctl`) | ||
| - Click "OK" on all windows | ||
|
|
||
| 4. **Restart your command prompt or PowerShell** for the changes to take effect. | ||
|
|
||
| </details> |
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 manual installation steps for Windows are a good start. As you asked for feedback on this, here are a couple of suggestions to improve the user experience:
-
Clarify Extraction Step: The instruction on line 179, "The executable will be in this folder (you may need to navigate into a subfolder)", is a bit vague. It would be more helpful to be specific about the structure of the ZIP file. For example: "After extraction, you will find
otdfctl.exeinside a folder likeotdfctl-0.28.0-windows-amd64. Moveotdfctl.exeto the%USERPROFILE%\otdfctlfolder you created." -
Automate with a Script: For a much smoother experience, you could provide a PowerShell script to automate the download, extraction, and PATH update. Users could run it with a single command. Here's an example:
# Set variables $version = "0.28.0" # This would be updated with releases $arch = "amd64" # or "arm64" $url = "https://github.com/opentdf/otdfctl/releases/download/v$version/otdfctl-$version-windows-$arch.zip" $installDir = "$env:USERPROFILE\otdfctl" $zipFile = "$env:TEMP\otdfctl.zip" # Create installation directory if it doesn't exist if (-not (Test-Path -Path $installDir)) { New-Item -ItemType Directory -Path $installDir | Out-Null } # Download and extract Write-Host "Downloading otdfctl v$version..." Invoke-WebRequest -Uri $url -OutFile $zipFile Write-Host "Extracting to $installDir..." Expand-Archive -Path $zipFile -DestinationPath $installDir -Force # Add to user's PATH if not already present $currentUserPath = [System.Environment]::GetEnvironmentVariable('Path', 'User') if ($currentUserPath -notlike "*$installDir*") { $newPath = "$currentUserPath;$installDir" [System.Environment]::SetEnvironmentVariable('Path', $newPath, 'User') Write-Host "Added '$installDir' to your PATH. Please restart your terminal for the changes to take effect." } else { Write-Host "'$installDir' is already in your PATH." } # Clean up Remove-Item $zipFile Write-Host "Installation complete."
-
Package Managers: Long-term, submitting
otdfctlto a package manager like Winget or Scoop would be ideal for simplifying installation and updates.
X-Test Failure Reportbats-test-results |
View the rendered file
View the new file (rendered) at the following link:
Looking for feedback
release-pleaseto update version numbers in "INSTALL.md" (this is my preference) or use placeholder version numbers / variables?Description
INSTALL.md, to provide installation instructions for theotdfctlcommand-line tool.