Skip to content
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

Simplify healthcheck build #2530

Closed
wants to merge 2 commits into from

Conversation

Jamie-
Copy link

@Jamie- Jamie- commented Jan 3, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Simplifies healthcheck build. Installing node into the Golang container just for scripting purposes adds a lot of time overhead to solve a simple problem. Bash is already available in the container and can easily do everything required.

I've tested the behaviour and is identical to the original JS implementation.

Type of change

Please delete any options that are not relevant.

  • Other - build optimisation

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

Copy link
Contributor

@Computroniks Computroniks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT

@louislam
Copy link
Owner

louislam commented Jan 5, 2023

Sorry, I was intended to write it in Javascript, just because I am not familiar with the syntax of bash/sh. I will be in trouble if I will modify it in the future.

Since it is not for end users and usually the installation of Node.js image layers are cached, it won't be too slow in my case.

So I chose JavaScript for better readability and maintainability.

@louislam louislam closed this Jan 5, 2023
@Jamie-
Copy link
Author

Jamie- commented Jan 5, 2023

Bash isn't a hard language to learn, there's plenty of help/tutorials/examples on the internet. Stack overflow has answers for pretty much everything you would ever want to do in bash. Also there's loads of people here who can help if really get stuck (looking at the PR/issues/star counts).

I don't feel that closing becuase you don't want to learn is a good reason, and hinders contributions to make a really cool tool even better.

The saying goes, 'use the right tool for the job' and JS is really not the right tool here.

I understand that for the majority of users, this is something they won't see. However there are a different subset of users, myself included, who want to control what code they run and so will wish to build the code and docker images themselves, and this does impact those users.

@louislam
Copy link
Owner

louislam commented Jan 5, 2023

Sorry, it is maybe my personal choice.

JS is really not the right tool here.

In my opinion, JS is the best tool here. All functions names are very clear and easy to understand and remember. -f, set -e, $# -ne 1 and "$1" they looks like alien to me.

Anyway, thanks for your work, I would recommend that we should have a discussion first as our pull request rules stated.

@Jamie-
Copy link
Author

Jamie- commented Jan 5, 2023

https://devhints.io/bash

Specifically:

-f -ne inside an if: https://devhints.io/bash#conditionals

$# $1: https://wiki.bash-hackers.org/syntax/shellvars#special_parameters_and_shell_variables

set -e: https://stackoverflow.com/questions/19622198/what-does-set-e-mean-in-a-bash-script

Google is your friend


However it's contradictory that you don't understand set -e given it's used here; https://github.com/louislam/uptime-kuma/blame/master/extra/entrypoint.sh#L3-L4

And you added a comment explaining what it does in f0ac3c8

Why are extra/entrypoint.sh and extra/upload-github-release-asset.sh in bash instead of JS if bash is not clear enough?

@louislam
Copy link
Owner

louislam commented Jan 5, 2023

No hard feelings. The main issue is not about Google. The issue is maybe after a year, I will completely forget what are the meaning of those syntax.

https://github.com/louislam/uptime-kuma/blame/master/extra/entrypoint.sh#L3-L4

Your example is good, I cannot believe I actually added a comment for set -e last year. And I forget what is it now, lol. That's why I don't like Bashscript.

JS here is very clear, fs.existsSync, fs.rmSync etc I will probably remember them in my life.

  • extra/entrypoint.sh: It is about changing user using setpriv, which is I think is the only choice. It will be dropped in 2.0.0 (See: https://github.com/louislam/uptime-kuma/pull/2086/files)
  • extra/upload-github-release-asset.sh: Just a script I downloaded from Internet under MIT license, I don't own the copyright. It just a program here. I never read the code and touch it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants