Add alpine cloudflared script set - #1360
Conversation
Greptile OverviewGreptile SummaryThis PR adds Alpine Linux support for Cloudflared with interactive token configuration. However, it violates a critical repository rule requiring all Key issues found:
The implementation itself appears functional with proper user configuration handling and service setup, but the OS specification violations are blocking issues that must be resolved before merge. Confidence Score: 1/5
Important Files Changed
|
| token_valid() { | ||
| local token="$1" | ||
|
|
||
| # Sanitize token of unprintable chars |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # Sanitize token of unprintable chars | ||
| token=$(echo "$token" | tr -cd '[:print:]') | ||
|
|
||
| # Validate token is present and alphanumeric (should be Base64) |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return 1 | ||
| fi | ||
|
|
||
| # export for use in install script |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| # Enable sysctl service so conf is applied on start | ||
| $STD rc-update add sysctl | ||
| # Increase ping_group_range by one to create space for cloudflared group |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| net.ipv4.ping_group_range = 65534 65535 | ||
| EOF | ||
|
|
||
| # Apply 90-cloudflared.conf now |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| # Apply 90-cloudflared.conf now | ||
| $STD sysctl -p /etc/sysctl.d/90-cloudflared.conf | ||
| # Create cloudflared group in ping_group_range |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| if [ -z "${TOKEN:-}" ]; then | ||
| mkdir -p "$CONFIG_PATH" | ||
| # Create empty config file so permissions are correct and users can find it |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| exit 1 | ||
| fi | ||
|
|
||
| # Start service now if externally managed, otherwise user needs to setup config first. |
There was a problem hiding this comment.
remove non-header comment
Context Used: Rule from dashboard - What: Shell script files in ct/ and /install directories must not contain comments except for file h... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
The whole Script is AI generated? Then use in Docs the AI.md and improve the Script. Many Issues or wrong conversations. All Scripts should have the Same Syntax. Alpine is very smart and should be an easy Script |
| @@ -0,0 +1,143 @@ | |||
| #!/usr/bin/env bash | |||
| # shellcheck source=misc/build.func | |||
There was a problem hiding this comment.
| # shellcheck source=misc/build.func |
There was a problem hiding this comment.
Without this shellcheck gives a warning. I also think it's nice that it points to the scripts location in the repo.
There was a problem hiding this comment.
We can disable the lint in the shellcheckrc if everyone thinks it's not useful.
| var_token="${var_token:-}" | ||
| var_config_path="${var_config_path:-/usr/local/etc/cloudflared}" |
There was a problem hiding this comment.
| var_token="${var_token:-}" | |
| var_config_path="${var_config_path:-/usr/local/etc/cloudflared}" |
Where did you get that from?
There was a problem hiding this comment.
I created them in case users want to set the var and skip the whiptail menu. I can make it a normal ENV var but figured the user choice would be nice and wanted to stick with the convention for user settable vars.
There was a problem hiding this comment.
We can also use them to skip the menu for CD/CI if you have a workflow that tests skip installation.
There was a problem hiding this comment.
Just stick to the standards here please.
None of this is AI generated. I customized the script to install a production ready container with user friendly setup. Is it the comments that give you that impression? I think code comments are important for maintainability and understanding.
Could you point out some of your issues with the script? Most variance comes from allowing the user to use a token (remote managed) or a config file (locally managed). That and setting up a cloudflared user with limited permissions, which is a security best practice. The cloudflared group should also be in the If your main issue is the whiptail menu, that is in the docs and I tried to keep it simple and added comments to explain what it's doing. |
| if [ -z "$var_token" ]; then | ||
| echo -e "${TAB}📝${TAB}${BOLD}${DGN}Tunnel Config: ${BGN}${CONFIG_PATH}/config.yml${CL}" | ||
| else | ||
| echo -e "${TAB}🪪${TAB}${BOLD}${DGN}Tunnel Token: ${BGN}${TOKEN:0:33}...${CL}" | ||
| fi |
There was a problem hiding this comment.
These lines can be removed but I thought it was helpful information for the user.
|
The frontend test are failing because I updated |
|
Yes because cloudflare isnt available here |
e942407 to
50d47dc
Compare
5e52452 to
b5171d2
Compare
acd2a49 to
67a4292
Compare
|
@bandogora This PR has been marked as stale. It will be closed if no new commits are added in 7 days. |
|
You are still missing to address some review comments from Greptile. Also can you please explaine the whole function in the main file. Why is that needed. This dose not stick to our standards. |
|
@bandogora Closing stale PR due to inactivity (no commits for 7 days after stale label). |
Scripts which are clearly AI generated and not further revised by the Author of this PR (in terms of Coding Standards and Script Layout) may be closed without review.
✍️ Description
Adds alpine-cloudflared script set and update frontend json
🔗 Related PR / Issue
Link: #
✅ Prerequisites (X in brackets)
🛠️ Type of Change (X in brackets)
README,AppName.md,CONTRIBUTING.md, or other docs.🔍 Code & Security Review (X in brackets)
Code_Audit.md&CONTRIBUTING.mdguidelinesAppName.sh,AppName-install.sh,AppName.json)📋 Additional Information (optional)
📦 Application Requirements (for new scripts)