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

feat: config supports toggling HTTP and Stats servers on/off (#594) #600

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jaw
Copy link

@jaw jaw commented Nov 14, 2024

This changes the behavior of the stats server and git server but I assume most people don't want that by default.
The ssh/http servers are most likely the primary ones?

Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

This looks good from at a first glance. We need to be able to disable/enable all of them including http/ssh. We shouldn't assume people won't use them, especially since this is the default behavior right now. So let's make them enabled by default.

pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
cmd/soft/serve/server.go Outdated Show resolved Hide resolved
cmd/soft/serve/server.go Outdated Show resolved Hide resolved
cmd/soft/serve/server.go Outdated Show resolved Hide resolved
cmd/soft/serve/server.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@jaw jaw force-pushed the main branch 2 times, most recently from 9df0211 to 45494f5 Compare November 14, 2024 15:22
Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

LGTM! Finally, could you add tests to check if the servers are enabled/disabled

@jaw
Copy link
Author

jaw commented Nov 18, 2024

LGTM! Finally, could you add tests to check if the servers are enabled/disabled

Not exactly sure what you're looking for...
Do you mean a test case where the env vars are verified to be working?

@aymanbagabas
Copy link
Member

Do you mean a test case where the env vars are verified to be working?

Yes. You could use testscripts to write the tests

@jaw
Copy link
Author

jaw commented Nov 19, 2024

Do you mean a test case where the env vars are verified to be working?

Yes. You could use testscripts to write the tests

Ack, done.

Tests got a little bit of polish as well.

@aymanbagabas
Copy link
Member

Tests got a little bit of polish as well.

Perfect! Could you put the test changes in a separate PR so that it doesn't get mixed up with the changes here?

Jonatan Wallmander added 4 commits November 21, 2024 10:13
This was inconsistent with the other servers.
The generic "waitforserver" has been renamed to
"ensureserverrunning".

This command now also takes an argument which denotes
which environment variable to pick the port from.

This is needed as the ports are randomized by the test.
This is useful on Windows where gcc is not always
available.
…#594)

To test this, tests added:

  `ensureservernotrunning [SERVICE_NAME]`
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