egor-tensin / setup-wireguard

GitHub action to set up WireGuard
MIT License
61 stars 21 forks source link

Extract script outside of action.yml + remove unused code + add test that does not rely on a remote wireguard server #8

Open Tycale opened 1 year ago

Tycale commented 1 year ago

Extracting the bash script outside of the yaml file helps in order to apply Shellcheck and other linters. It showed no errors, so that's great ! I removed the unused code (about systemD) as I think it's a bit confusing. I changed the array call for the additional arguments. I pushed everything in a single commit, sorry for that.

Thanks for this helpful Github action.

Tycale commented 1 year ago

Crazy, again this error ! I would love to see the output with -o xtrace but this will leak the IPs you are using in the workflow run. So I won't. I will add an "echo" instead at the array expansion.

Tycale commented 1 year ago

Force pushing does not re-run the workflow.. Too bad :p

Tycale commented 1 year ago

I don't get it, it is running without issue on my GHA "runs-on: ubuntu-latest"..

Tycale commented 1 year ago

So issue is most likely at line 72 https://github.com/egor-tensin/setup-wireguard/pull/8/files#diff-c992f6abfc7ff51f20d8266ec1acff44ac43ff1e87bcc75968bc2b9f0b72da23R72

As this is the only change I made in this file (with the management of the variables). But I really don't understand why. Crazy bash array expansion.

egor-tensin commented 1 year ago

So issue is most likely at line 72 https://github.com/egor-tensin/setup-wireguard/pull/8/files#diff-c992f6abfc7ff51f20d8266ec1acff44ac43ff1e87bcc75968bc2b9f0b72da23R72

As this is the only change I made in this file (with the management of the variables). But I really don't understand why. Crazy bash array expansion.

Please see a page on my website about bash: https://egort.name/blog/notes/bash.html. Especially under "Arrays" -> "Expansion". Basically, the rule is: always expand arrays using ${xs[@]+"${xs[@]}"}. I agree that this is crazy. Please revert this change about array expansion.

Tycale commented 1 year ago

I was not previously aware of the ${xs[@]+"${xs[@]}"} syntax in Bash. This seems unusual but I ain't try fight with Bash.

Tycale commented 1 year ago

I think the error is triggered by the fact that the vars are missing !

image

And the bash error:

Error: any valid address is expected rather than "dev".

It is coming from the line 51 : https://github.com/egor-tensin/setup-wireguard/pull/8/files#diff-c992f6abfc7ff51f20d8266ec1acff44ac43ff1e87bcc75968bc2b9f0b72da23R51

With ifname defined but not ip:

# ip addr add "$ip" dev "$ifname"
Error: any valid prefix is expected rather than "".
Tycale commented 1 year ago

Can you try to run the workflow manually from your side ? https://github.com/egor-tensin/setup-wireguard/blob/master/.github/workflows/test.yml#L26

Does Github keeps the secrets empty when it's executed by a pull request ? The only var going through is the keepalive with the value 25..

Tycale commented 1 year ago

@egor-tensin What do you think about this change ? This makes the test self-contained thanks to network namespaces.