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

test: add LIVEPEER_HTTP_PORT test env variable #2936

Closed
wants to merge 2 commits into from

Conversation

rickstaa
Copy link
Member

@rickstaa rickstaa commented Dec 16, 2023

This pull request adds the LIVEPEER_HTTP_PORT environment variable, which can set the HTTP port used by the Livepeer binary in the test_args.sh script. This is useful if another service already uses this port.

What does this pull request do? Could you explain your changes? (required)

I already have an orchestrator running on the default port. This port is therefore not available to be used for the tests.

Specific updates (required)

  • Adds the LIVEPEER_HTTP_PORT environment variable, which can set the HTTP port used by the Livepeer binary in the test_args.sh script.

How did you test each of these updates (required)

By setting LIVEPEER_HTTP_PORT environment variable and running the test_args.sh script.

Does this pull request close any open issues?

Checklist:

@rickstaa rickstaa changed the title test: add LIVEPEER_HTTP_PORT test env variable test: add LIVEPEER_HTTP_PORT test env variable Dec 16, 2023
This commit adds the `LIVEPEER_HTTP_PORT` environment variable, which
can set the HTTP port used by the Livepeer binary in the `test_args.sh`
script. This is useful if another service already uses this port.
@rickstaa
Copy link
Member Author

Should we also offer a method to modify the CLI port? While reviewing the code, I couldn't locate any tests that interact with the CLI. It would be greatly appreciated if someone could validate this and provide confirmation.

@leszko
Copy link
Contributor

leszko commented Dec 18, 2023

@rickstaa Thanks for the PR.

I wonder how much useful this feature is. AFAIK test_args.sh is just a test. If it collides with your other O running on the same machine, maybe you can just run the test inside a Docker container? Or run it on a difference machine?

@rickstaa
Copy link
Member Author

@rickstaa Thanks for the PR.

I wonder how much useful this feature is. AFAIK test_args.sh is just a test. If it collides with your other O running on the same machine, maybe you can just run the test inside a Docker container? Or run it on a difference machine?

I'm running my Orchestrator, a Docker container, while exposing the same ports. Unfortunately, I've encountered issues where the tests fail to run successfully on my other machines (you can find more details here: #2905 (comment)).

While I can run the tests in a Docker container, I wanted to find a more convenient way to test my changes, so I created this pull request. The decision to include this environmental variable in your codebase is up to you. Feel free to close it if you prefer developers to run the tests in a docker container when the port is already in use 👍🏻.

@leszko
Copy link
Contributor

leszko commented Dec 19, 2023

@rickstaa Thanks for the PR.
I wonder how much useful this feature is. AFAIK test_args.sh is just a test. If it collides with your other O running on the same machine, maybe you can just run the test inside a Docker container? Or run it on a difference machine?

I'm running my Orchestrator, a Docker container, while exposing the same ports. Unfortunately, I've encountered issues where the tests fail to run successfully on my other machines (you can find more details here: #2905 (comment)).

While I can run the tests in a Docker container, I wanted to find a more convenient way to test my changes, so I created this pull request. The decision to include this environmental variable in your codebase is up to you. Feel free to close it if you prefer developers to run the tests in a docker container when the port is already in use 👍🏻.

Yeah, I'm a little hesitant to adding more "features" when not needed. Since we have all the other ports hardcoded (and I'm actually we need them configurable), I'd probably stick to what we have. Unless we find more use-cases why to configure ports.

@rickstaa
Copy link
Member Author

@leszko, although the environment variable would have improved my developer experience, I understand your decision 👍🏻. Let's close this for now. I will use docker instead and add the steps to #2937.

@rickstaa rickstaa closed this Dec 19, 2023
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.

2 participants