search

robinmanuelthiel / speedtest

Check internet bandwidth from a Docker container and save the results to an InfluxDB
MIT License
176 stars 47 forks source link

Extended to set custom Server ID #16

Closed madnuttah closed 4 months ago

madnuttah commented 8 months ago

Script and yaml files edited, I hope it's fine.

madnuttah commented 8 months ago

The concerns regarding the wait command could be circumvented by (mis)using a healthcheck-script like I did here

madnuttah commented 8 months ago

My apologies, apparently this is new to me.

bazzani commented 5 months ago

Hi @robinmanuelthiel and @madnuttah

I have been looking at this repo too and I want to introduce a similar change to supply a hostname (with -o argument) to the speedtest command optionally, and I plan to open a PR soon from my fork.

Concerning this PR, I tested your changes locally @madnuttah and I observed a problem where the docker container fails if I do not specify the server ID. I think it would be good to make this ability to add a server ID optional, and if it is missing the the regular speedtest CLI command should be invoked without the -s argument.

Container logs:

Running a Speed Test...
2024-05-30T07:41:46.102927881Z Thu May 30 07:41:46 2024
2024-05-30T07:41:46.107768923Z speedtest: Missing argument for option: -s
2024-05-30T07:41:46.149114881Z Running next test in 1800s...
2024-05-30T07:41:46.149116256Z

p.s. the similar change I made can be found at

robinmanuelthiel commented 5 months ago

Yeah, if we introduce Server ID and URL, I would like to see them as optional parameters as well. This implementation here requires them.

robinmanuelthiel commented 5 months ago

Hi @robinmanuelthiel and @madnuttah

I have been looking at this repo too and I want to introduce a similar change to supply a hostname (with -o argument) to the speedtest command optionally, and I plan to open a PR soon from my fork.

Concerning this PR, I tested your changes locally @madnuttah and I observed a problem where the docker container fails if I do not specify the server ID. I think it would be good to make this ability to add a server ID optional, and if it is missing the the regular speedtest CLI command should be invoked without the -s argument.

Container logs:

Running a Speed Test...
2024-05-30T07:41:46.102927881Z Thu May 30 07:41:46 2024
2024-05-30T07:41:46.107768923Z speedtest: Missing argument for option: -s
2024-05-30T07:41:46.149114881Z Running next test in 1800s...
2024-05-30T07:41:46.149116256Z

p.s. the similar change I made can be found at

This looks promising. Can you create a PR from your repo back into this one?

bazzani commented 5 months ago

p.s. the similar change I made can be found at

This looks promising. Can you create a PR from your repo back into this one?

Hi @robinmanuelthiel, may I ask you to please clarify that you are referring to my suggested change?

If so, I can work on this, and also add support for the -s flag at the same time although providing them must be mutually exclusive or this causes an error when running the speedtest cli with both -o and -s provided 👇

robinmanuelthiel commented 5 months ago

Yes, exactly. To me it's important, that I also works without specifying any server name or IP.

bazzani commented 5 months ago

Yes, exactly. To me it's important, that I also works without specifying any server name or IP.

100%, I will open the PR soon enough, after giving it a thorough test locally.

bazzani commented 5 months ago

I also have another WIP change in a feature branch that uses the Dockerfile to build the speedtest image in the docker compose file, rather than the image being pulled from dockerhub. Is this something you would consider reviewing @robinmanuelthiel?

robinmanuelthiel commented 5 months ago

@bazzani Not sure, if I like the build part in the Docker Compose file, as I wanted to give an example, how to use the container easily in Docker Compose file. Building it yourself, would make it harder. But we could maybe add another examle file for self-building or just use both, image and build https://docs.docker.com/compose/compose-file/build/#using-build-and-image

robinmanuelthiel commented 5 months ago

Hey there and sorry for my late reply. I must confess this was quite rushed. It was working so I submitted the PR without any further tests, as I wrote it did what I wanted. I'm sorry if it didn't meet the quality you've expected. As I'm doing much better again, I'm getting a take on this again.

Hey no worries - thanks for the contribution. We are working on this together! 💪

bazzani commented 5 months ago

@bazzani Not sure, if I like the build part in the Docker Compose file, as I wanted to give an example, how to use the container easily in Docker Compose file. Building it yourself, would make it harder. But we could maybe add another examle file for self-building or just use both, image and build https://docs.docker.com/compose/compose-file/build/#using-build-and-image

I see what you mean @robinmanuelthiel, thanks for the input and helping me consider the other optionsyou proposed. I think I was perhaps a tad too deep in the developer mindset, and not enough in a consumer (of the service) mindset.

I will review that link you sent and play about a bit with the image and build approach.

bazzani commented 5 months ago

Hey there and sorry for my late reply. I must confess this was quite rushed. It was working so I submitted the PR without any further tests, as I wrote it did what I wanted. I'm sorry if it didn't meet the quality you've expected. As I'm doing much better again, I'm getting a take on this again.

Thank you for the PR @madnuttah, I do appreciate your changes as it helped me to understand the project more, and made me more curious as to how we can extend it by working together. OSS is the best!

madnuttah commented 4 months ago

@bazzani it seems like you invested way more time into this and the script looks better already than what I have here right now. I'll close this, also because of a lack of time. Sorry, I shouldn't have done this that time.

robinmanuelthiel commented 4 months ago

Sorry, I shouldn't have done this that time.

I disagree! Your effort kicked it all off. Can't wait to see this feature here soon. Thanks for your work @madnuttah! Keep it coming. (both feedback, issues and code contributions)

robinmanuelthiel commented 4 months ago

@bazzani, are you still planning to create a PR for the Server ID?

bazzani commented 4 months ago

@bazzani, are you still planning to create a PR for the Server ID?

Hi @robinmanuelthiel i am indeed. I started working on it last week and got stuck on a few shell scripting logic challenges (due to the two mutually exclusive optional flags). I then got sick on Friday so I have not had a chance to complete it yet.

I will look at it tomorrow and get something in a PR for your review.

bazzani commented 4 months ago

FYI @robinmanuelthiel @madnuttah PR opened #19