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

fix: modify the config default value keep consistency #1352

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

jujiale
Copy link
Contributor

@jujiale jujiale commented Dec 2, 2024

this is a config style optimize, nothing to do with core logic.
In config.go
there are some configs have default value, such as the following port 8082 and 8379
`

      type http struct {
      Enabled bool `config:"enabled" default:"true"`
      Port    int  `config:"port" default:"8082" validate:"number,gte=0,lte=65535"`
  }
  
  type websocket struct {
      Enabled                 bool          `config:"enabled" default:"true"`
      Port                    int           `config:"port" default:"8379" validate:"number,gte=0,lte=65535"`
      MaxWriteResponseRetries int           `config:"max_write_response_retries" default:"3" validate:"min=0"`
      WriteResponseTimeout    time.Duration `config:"write_response_timeout" default:"10s"`
  }

`

but in cli.go the default value is not the same, such as the following port 7380 and 7381
`

    flag.IntVar(&flagsConfig.HTTP.Port, "http-port", 7380, "port for accepting requets over HTTP")
    flag.BoolVar(&flagsConfig.HTTP.Enabled, "enable-http", false, "enable DiceDB to listen, accept, and process HTTP")

    flag.IntVar(&flagsConfig.WebSocket.Port, "websocket-port", 7381, "port for accepting requets over WebSocket")

`

this pr only keep them the same so that default config value keep consistency.

@jujiale
Copy link
Contributor Author

jujiale commented Dec 2, 2024

I think the test case failed has nothing to do with this pr, please have a check, thanks.

@piyushhhxyz
Copy link
Contributor

@jujiale sync latest changes, should be solved.

@jujiale
Copy link
Contributor Author

jujiale commented Dec 3, 2024

@jujiale sync latest changes, should be solved.

done

Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

LGTM

@lucifercr07 lucifercr07 merged commit 6b9d323 into DiceDB:master Dec 3, 2024
2 checks passed
Dev79844 pushed a commit to Dev79844/dice that referenced this pull request Dec 5, 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.

3 participants