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: Log bind address at startup #446

Merged
merged 1 commit into from
Jul 25, 2024
Merged

feat: Log bind address at startup #446

merged 1 commit into from
Jul 25, 2024

Conversation

stbrody
Copy link
Collaborator

@stbrody stbrody commented Jul 24, 2024

With this change at startup there's a message like

  2024-07-24T21:40:00.705970Z  INFO ceramic_one: starting api server at address 127.0.0.1:5101
    at one/src/lib.rs:575

It does make me wonder though if we should separate bind_address into separate args for the port to use and the network interface to bind to. It's kind of weird to print 127.0.0.1:5101 when presumably we also accept connections that don't just come from localhost. Really it's the port that's going to be the most interesting part for most people

@stbrody stbrody self-assigned this Jul 24, 2024
@stbrody stbrody requested a review from a team as a code owner July 24, 2024 21:42
Copy link

linear bot commented Jul 24, 2024

@stbrody stbrody requested a review from dav1do July 24, 2024 21:42
@stbrody stbrody temporarily deployed to github-tests-2024 July 24, 2024 21:53 — with GitHub Actions Inactive
Copy link
Contributor

@dav1do dav1do left a comment

Choose a reason for hiding this comment

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

For some reason I thought we already did this but it's only the p2p address. I think accepting the socket address as one parameter is fine personally, but it might be nice to actually make the CLI flag a SocketAddr instead of a string. It doesn't really change much, other than getting an error earlier e.g.

❯ ./target/debug/ceramic-one daemon --network local --local-network-id 0 --bind-address 127.0.0:5101
  2024-07-25T17:43:18.790913Z  INFO ceramic_one: service__name: "ceramic-one", version: "0.29.0", build: "git:570d299f", instance_id: "beautiful-able", exe_hash: "uAAUAdebugg"
    at one/src/lib.rs:343

...

  2024-07-25T17:43:18.974446Z  INFO ceramic_api::server: Shutting down insert task.
    at api/src/server.rs:347

  2024-07-25T17:43:18.974709Z ERROR ceramic_one: Error running command: invalid socket address syntax
    at one/src/main.rs:15

Error: invalid socket address syntax

vs

❯ ./target/debug/ceramic-one daemon --network local --local-network-id 0 --bind-address 127.0.0:5101
error: invalid value '127.0.0:5101' for '--bind-address <BIND_ADDRESS>': invalid socket address syntax

@stbrody
Copy link
Collaborator Author

stbrody commented Jul 25, 2024

yeah that does look like a nice improvement

@stbrody stbrody added this pull request to the merge queue Jul 25, 2024
Merged via the queue into main with commit 0dde02f Jul 25, 2024
5 checks passed
@stbrody stbrody deleted the logApiPort-AES-252 branch July 25, 2024 19:03
@smrz2001 smrz2001 mentioned this pull request Jul 29, 2024
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