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

[Feature] Added support for YAML/YML config file format and configurable server log output to file #525

Merged
merged 10 commits into from
May 10, 2022

Conversation

ZeljkoBenovic
Copy link
Contributor

@ZeljkoBenovic ZeljkoBenovic commented Apr 27, 2022

Description

This PR introduces the support for starting the server with configuration file written in YAML syntax.
Added the example-config.yaml file with minimal configuration as a starting point.

Additionally, this PR introduces a new server flag --log-to which provides the operator with an option to store all server output to a log file, instead of only having console logs.

Added the make test option to the Makefile for easy testing.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Modify per your environment and use the example-config.yaml file as an argument to the server --config flag to start the server.
When starting the server set --log-to <file_name> flag have all logs written in a file. With this flag set, the server command will not have any console outputs.

Fixes EDGE-39

* added --log-to server flag
* added newLogger and setLogFileWriter func
* logger instnce set to write log output to file if defined
* added test to Makefile
* added support for yaml files
* added example-config.yaml file with minimal config
Makefile Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@Kourin1996
Copy link
Contributor

Confirmed it worked locally

Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

LGTM

server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
example-config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Love this feature 💯

Looks great, I've left a few comments 🚀

* created new func newFileLogger and newCLILogger
* renamed newLogger to newLoggerFromConfig
* using params.rawConfig.LogFilePath to store flag value
* server export command exports config file with default values in yaml format
* changed server command flags default values to pull from DefaultConfig() instead of hardcoded
* server command now requires --data-dir flag to be set to some string value because server init requires it
* removed DefaultMaxSlots const as this value is now pulled from DefaultConfig()
Copy link
Contributor

@dbrajovic dbrajovic left a comment

Choose a reason for hiding this comment

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

Let's resolve the minor refactor suggestions and we can merge this 👍

* server export command moved to dedicated package
* added success message to the output
* added support for exporting config file to json format
@ZeljkoBenovic ZeljkoBenovic force-pushed the feature/configurable_log_output branch from 0f871fc to 9350d7d Compare May 7, 2022 01:59
@ZeljkoBenovic ZeljkoBenovic merged commit dd1d9af into develop May 10, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2022
@zivkovicmilos zivkovicmilos deleted the feature/configurable_log_output branch May 11, 2022 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New update to Polygon Edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants