Skip to content

Conversation

@Daniel1984
Copy link

@Daniel1984 Daniel1984 commented Oct 21, 2022

  • adding check to verify that actual config has same object keys as config example to avoid running workers with missing configuration.
  • added optional validation parameter
  • added test coverage
  • added linter and fixed few linter warnings
  • should we remove package-lock.json from .gitignore?
  • not using any 3rd party packages as Joi or similar to avoid extra dependencies and our comparison is pretty simple here
  • more info in README.md

@Daniel1984 Daniel1984 requested a review from ceecko November 21, 2022 12:58
Copy link

@ceecko ceecko left a comment

Choose a reason for hiding this comment

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

I left a comment in the nested keys example where I think the validation fails.

One more thing I just realized, not really a request for a change, rather a general comment - how do we want to approach optional configuration?

Let's say we have a configuration such as enableSlack: bool.
If it's enabled, the slackAPIKey needs to be populated, otherwise it can be left empty.
The example config shows an example for the slackAPIKey (as expected) but it's not mandatory to be populated in the config if Slack is disabled. This way the validation reports the API key is missing, although it's not mandatory.

It's not clear to me what the best approach would be here. I just wanted to bring this up since there can be this type of configuration options.

@ceecko
Copy link

ceecko commented Nov 21, 2022

One other topic - how do we want to approach checking configs which are based on a common config?
Such as ERC20 tokens having one example config but each token having a separate config of its own.

@Daniel1984
Copy link
Author

I would get back to the original issue we had: if there's a example config, we must ensure all fields are present in config. That is covered and we can improve moving forward

@Daniel1984
Copy link
Author

I left a comment in the nested keys example where I think the validation fails.

One more thing I just realized, not really a request for a change, rather a general comment - how do we want to approach optional configuration?

Let's say we have a configuration such as enableSlack: bool. If it's enabled, the slackAPIKey needs to be populated, otherwise it can be left empty. The example config shows an example for the slackAPIKey (as expected) but it's not mandatory to be populated in the config if Slack is disabled. This way the validation reports the API key is missing, although it's not mandatory.

It's not clear to me what the best approach would be here. I just wanted to bring this up since there can be this type of configuration options.

In config:

{
  "slackAPIKey": "xyz"
}

In implementation:

this.loadConf('some.coin', 'coin', { slackAPIKey: { required: true } })

// somewhere else:
if (this.ctx.conf.slackAPIKey) {
  this.ctx.lib.slackClient.send();
}

There's always more than one way to achieve things

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