malhuda/lookingglass
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 16 Wiki Activity

Docker Fixes, iPerf3 Feature Completion, Minor PHP Code Improvements #41

Merged
RealBrandon merged 8 commits from contrib into main 2024-04-28 00:47:51 +08:00
Conversation 13 Commits 8 Files Changed 8 +46 -17
RealBrandon commented 2024-04-20 06:43:23 +08:00 (Migrated from github.com)
Copy Link
  1. Docker Fixes
  • Add iPerf3 container.
  • Make containers' build paths consistent.
  • Fix latency detection not working under Docker environments:

This feature relies on ss which did not work when using Docker due to missing iproute2 package in php-fpm Docker image and incorrect network mode for the containers. ss relies on socket descriptors which do not exist in containers unless using host network mode. Plus, host network mode improves network performance when using Docker, especially beneficial to iPerf3 container.

  1. Minor PHP Code Improvements
  • When LG_LOCATIONS contains the current location, which in my opinion is expected when deploying in multiple locations. The location drop-down menu displays the same location twice due to $templateData['current_location'] and $templateData['locations'] both being displayed. An if check is implemented to deduplicate.
  • Default values for certain constants are changed to empty arrays to trigger auto-hiding by default. The previous placeholder values are kept in comments for users' reference.
  • Change "Test IPv4" and "Test IPv6" to "Looking Glass IPv4" and "Looking Glass IPv4", respectively, which in my opinion, is less ambiguous.
  • iPerf3 block now uses LG_IPV4 to display the IP address of the looking glass, instead of being set manually.
  1. Documentation
  • Add iPerf3-related instructions for manual installation.

There might also be some other small changes I failed to mention. Thank you for reading this.

1. Docker Fixes - Add iPerf3 container. - Make containers' build paths consistent. - Fix latency detection not working under Docker environments: > This feature relies on `ss` which did not work when using Docker due to missing `iproute2` package in `php-fpm` Docker image and incorrect network mode for the containers. `ss` relies on socket descriptors which do not exist in containers unless using `host` network mode. Plus, `host` network mode improves network performance when using Docker, especially beneficial to iPerf3 container. 2. Minor PHP Code Improvements - When `LG_LOCATIONS` contains the current location, which in my opinion is expected when deploying in multiple locations. The location drop-down menu displays the same location twice due to `$templateData['current_location']` and `$templateData['locations']` both being displayed. An `if` check is implemented to deduplicate. - Default values for certain constants are changed to empty arrays to trigger auto-hiding by default. The previous placeholder values are kept in comments for users' reference. - Change "Test IPv4" and "Test IPv6" to "Looking Glass IPv4" and "Looking Glass IPv4", respectively, which in my opinion, is less ambiguous. - iPerf3 block now uses `LG_IPV4` to display the IP address of the looking glass, instead of being set manually. 3. Documentation - Add iPerf3-related instructions for manual installation. There might also be some other small changes I failed to mention. Thank you for reading this.
dqos commented 2024-04-22 17:22:48 +08:00 (Migrated from github.com)
Copy Link

@RealBrandon Thank you for your PR, I will go through this asap. I have some changes I would like to see before we merge this 👍🏽

@RealBrandon Thank you for your PR, I will go through this asap. I have some changes I would like to see before we merge this 👍🏽
👍 1
RealBrandon commented 2024-04-22 20:59:33 +08:00 (Migrated from github.com)
Copy Link

Hi there, this is the first time I am contributing to a public project. If there is anything that doesn't conform to the convention, please let me know. I'll be more than willing to change it. Thank you for going through my code.

Hi there, this is the first time I am contributing to a public project. If there is anything that doesn't conform to the convention, please let me know. I'll be more than willing to change it. Thank you for going through my code.
🎉 1
dqos (Migrated from github.com) reviewed 2024-04-25 03:30:09 +08:00
dqos (Migrated from github.com) left a comment
Copy Link

Nice first public PR, good job. I have done a short review on your code.
Did you test the Docker part? If it works then that part is good to go.

Nice first public PR, good job. I have done a short review on your code. Did you test the Docker part? If it works then that part is good to go.
config.dist.php
dqos (Migrated from github.com) commented 2024-04-25 03:24:37 +08:00
Copy Link

This should not change, because not everybody runs the iperf server on the looking glass server.

This should not change, because not everybody runs the iperf server on the looking glass server.
dqos (Migrated from github.com) commented 2024-04-25 03:24:47 +08:00
Copy Link

Same here ^

Same here ^
docker/php-fpm/src/config.php
dqos (Migrated from github.com) commented 2024-04-25 03:26:04 +08:00
Copy Link

Why remove the examples? Easier to keep them commented to show people how it works. Remember that most people installing such looking glass aren't that technical in terms of coding 😄

Why remove the examples? Easier to keep them commented to show people how it works. Remember that most people installing such looking glass aren't that technical in terms of coding 😄
dqos (Migrated from github.com) commented 2024-04-25 03:26:53 +08:00
Copy Link

Don't forget to change it here too.

Don't forget to change it here too.
dqos (Migrated from github.com) commented 2024-04-25 03:27:01 +08:00
Copy Link

Here too.

Here too.
dqos (Migrated from github.com) commented 2024-04-25 03:29:19 +08:00
Copy Link

Keep the examples to show how it works.

Keep the examples to show how it works.
dqos (Migrated from github.com) reviewed 2024-04-25 03:32:36 +08:00
README.md
dqos (Migrated from github.com) commented 2024-04-25 03:32:35 +08:00
Copy Link

Can you put the iPerf installation guide in a separate section? As mentioned earlier, this makes the installation more complex and not everybody runs an iPerf server. If you do run it, usually you do this on a separate server due to congestion (not exactly ideal for a looking glass).

Can you put the iPerf installation guide in a separate section? As mentioned earlier, this makes the installation more complex and not everybody runs an iPerf server. If you do run it, usually you do this on a separate server due to congestion (not exactly ideal for a looking glass).
RealBrandon commented 2024-04-26 02:52:18 +08:00 (Migrated from github.com)
Copy Link

Nice first public PR, good job. I have done a short review on your code. Did you test the Docker part? If it works then that part is good to go.

Thank you very much. As for the Docker part, yes, I have tested them. I've deployed them in production environment as well. Here is the link to one of the looking glass I've deployed.

> Nice first public PR, good job. I have done a short review on your code. Did you test the Docker part? If it works then that part is good to go. Thank you very much. As for the Docker part, yes, I have tested them. I've deployed them in production environment as well. Here is the [link](http://sjc.lg.salmoncloud.co.uk/) to one of the looking glass I've deployed.
👍 1
RealBrandon (Migrated from github.com) reviewed 2024-04-26 02:54:45 +08:00
config.dist.php
RealBrandon (Migrated from github.com) commented 2024-04-26 02:54:44 +08:00
Copy Link

I didn't think of this scenario at all. Sorry, I'll change these back and other corresponding parts as well.

I didn't think of this scenario at all. Sorry, I'll change these back and other corresponding parts as well.
RealBrandon (Migrated from github.com) reviewed 2024-04-26 02:58:06 +08:00
README.md
RealBrandon (Migrated from github.com) commented 2024-04-26 02:58:06 +08:00
Copy Link

Yeah, no problem. By the way, would it be good if I comment out iPerf3-related configurations in docker-compose.yml? This way, when the user installs via Docker, it doesn't come with iPerf3 by default, since usually it should be deployed separately.

Yeah, no problem. By the way, would it be good if I comment out iPerf3-related configurations in `docker-compose.yml`? This way, when the user installs via Docker, it doesn't come with iPerf3 by default, since usually it should be deployed separately.
RealBrandon (Migrated from github.com) reviewed 2024-04-26 03:02:00 +08:00
docker/php-fpm/src/config.php
RealBrandon (Migrated from github.com) commented 2024-04-26 03:02:00 +08:00
Copy Link

Sorry I thought changing the default value to empty would allow the location drop-down menu to auto hide out of the box. I did keep them in the comments though. Should I change these back?

Sorry I thought changing the default value to empty would allow the location drop-down menu to auto hide out of the box. I did keep them in the comments though. Should I change these back?
RealBrandon (Migrated from github.com) reviewed 2024-04-26 03:02:44 +08:00
docker/php-fpm/src/config.php
RealBrandon (Migrated from github.com) commented 2024-04-26 03:02:44 +08:00
Copy Link

Roger that. I will change them back.

Roger that. I will change them back.
dqos (Migrated from github.com) reviewed 2024-04-26 16:07:03 +08:00
README.md
dqos (Migrated from github.com) commented 2024-04-26 16:07:03 +08:00
Copy Link

Yes, good idea! We need to keep as much as possible optional, and very easy too.

Yes, good idea! We need to keep as much as possible optional, and very easy too.
RealBrandon commented 2024-04-27 23:04:59 +08:00 (Migrated from github.com)
Copy Link

@dqos Hi, I've changed back the iPerf3 commands and the default values of the constants. Also, I've separated the iPerf3 instructions into a different section. Please have a look when you're free. Have a good weekend.

@dqos Hi, I've changed back the iPerf3 commands and the default values of the constants. Also, I've separated the iPerf3 instructions into a different section. Please have a look when you're free. Have a good weekend.
dqos commented 2024-04-28 00:47:48 +08:00 (Migrated from github.com)
Copy Link

@RealBrandon Looks good, thank you. Merging this into main, congratulations with your first contribution!

@RealBrandon Looks good, thank you. Merging this into main, congratulations with your first contribution!
❤️ 1
RealBrandon commented 2024-04-28 00:50:35 +08:00 (Migrated from github.com)
Copy Link

@RealBrandon Looks good, thank you. Merging this into main, congratulations with your first contribution!

Thank you so much. Looking forward to working with you in the future!

> @RealBrandon Looks good, thank you. Merging this into main, congratulations with your first contribution! Thank you so much. Looking forward to working with you in the future!
🚀 1
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: malhuda/lookingglass#41
No description provided.
Delete Branch "contrib"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?