- Notifications
You must be signed in to change notification settings - Fork72
refactor: refactored get_http_dir#360
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 fixes a bash syntax error in theget_http_dir() function that was preventing the script from executing properly on certain bash versions. The original code used bash array syntax that was causing parsing errors.
- Simplified the bash script logic by replacing array-based parsing with grep and awk
- Updated module version from 1.2.2 to 1.2.3
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| registry/coder/modules/kasmvnc/run.sh | Refactored get_http_dir function to use grep+awk instead of bash arrays |
| registry/coder/modules/kasmvnc/README.md | Updated version number to reflect the bug fix |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The awk-based YAML parsing improvement needed proper escaping of fieldreferences as 2936092 so Terraform's templatefile() function processes themcorrectly. This resolves template syntax errors that were causing theDateTime.pm installation failures.
…or Ubuntu 24.04 bug
@matifali I noticed a bug with ubuntu 24.04 so I added something small to resolve that. This is tested and working. Once you approve I will go ahead and release it |
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 Both
Closes #
Description
I just couldn't get the script to execute properly in its current form. I saw e.g.
[[: 1989{#d[@]}: syntax error: invalid arithmetic operator (error token is "{#d[@]}")when trying to run the script locally. (GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu)).
This uses a likely simpler bash script, but requires both grep and awk.
Type of Change
Module Information
Path:
registry/coder/modules/kasmvncNew version:
v1.2.3Breaking change: [ ] Yes [x] No
Testing & Validation
bun test)bun run fmt)Related Issues