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

Change the mechanism of reading configuration as well as the style of it #879

Merged
merged 17 commits into from
Jul 8, 2020

Conversation

ajuszczak
Copy link
Contributor

@ajuszczak ajuszczak commented Jul 8, 2020

Originally taken from: #875 but I decided to create another branch due to my mistake while doing rebase

This pull-request updates the style of the configuration, which is needed to improve way of passing custom properties to configuration file from the docker. This change is needed to add support for multiple blockchains.

In this change I wanted to address couple of improvements that can be done to address this issue:

a) We should not use 'sed' from shell, to replace our own macros with environment variables. Typesafe-config already supports this feature.
b) We should not copy entire configuration and fill up all of the variables, if user wants to update only couple of them.
c) Default variables should be stored in classpath's configuration, not somewhere in shell script (due to above mechanism).
d) There should be an option to run single and/or multiple blockchains for the single Conseil API instance.

All of this can be solved by using specific macro from typesafe-config. Sadly, this macro does not work when section of configuration is dynamic. Meaning:
Works:

{
  name: ${?CONSEIL_BLOCKCHAIN_NAME}
}

Won't work:

${?CONSEIL_BLOCKCHAIN_NAME} {
}

Sadly, the latter is how we have it right now. I found that not that intuitive as originally thought. Most of the things can be done by implementing this configuration as a list, instead of map.

My idea is to change the configuration into something like that:

platforms: [
  {
    name: "tezos"

    network: "zeronet"
    network: ${?CONSEIL_API_TEZOS_NETWORK}

    enabled: false
    enabled: ${?CONSEIL_API_TEZOS_ENABLED}

    node: {
      protocol: "https"
      protocol: ${?CONSEIL_API_TEZOS_NODE_PROTOCOL}

      hostname: "nautilus.cryptonomic.tech"
      hostname: ${?CONSEIL_API_TEZOS_NODE_HOSTNAME}

      port: 8732
      port: ${?CONSEIL_API_TEZOS_NODE_PORT}

      path-prefix: "tezos/zeronet/"
      path-prefix: ${?CONSEIL_API_TEZOS_NODE_PATH_PREFIX}
    }
  }
]
  • That change would give us better flexibility when it comes to deployment (cause you can see already, that these ${} are macros that can be filled from environment variables).
  • That change simplify the code of reading the configuration (at least I want to change the case-classes a bit and use as much of pureconfig mechanisms as we can, to make is easier to maintain and understand).
  • That change simplify the docker scripts (probably we will be able to get rid of most of the things from /docker/ folder).
  • That change create single point, where configuration will be stored with it's defaults. Meaning that only classpath's configuration should contain default values.
  • That change make the docker-compose file to specify only properties we want to overwrite, not all of them.
  • During this process I'll also increase version of pureconfig, cause current one is quite old (0.11 vs 0.13 where significant change has been made). [won't be done]
  • Additionally, default naming convention has been unified where possible from CamelCase to kebab-case.

Sadly, I tried to update pureconfig but I there is significant change in a way of reading configuration which overcomplicates things. I guess they forgot about one mechanism and it will be implemented in a future. I'll try to address that issue nonetheless but in upcoming days.

This pull-requests addresses all of the mentioned points.

Code review:

  • Update documentation for PlatformsConfiguration
  • Remove Unknown blockchain type and assign different one for ArgumentsConfig
  • Remove HOCON macros for the properties we don't want to easily expose (these, that won't work without coding anyway)
  • Change key in application.conf from CONSEIL_API_SECURITY_KEY to CONSEIL_API_KEY
  • Change key in application.conf from CONSEIL_API_SECURITY_ALLOW_BLANK to CONSEIL_API_ALLOW_BLANK_KEYS

@ajuszczak ajuszczak added the Btc-Int Bitcoin Integration label Jul 8, 2020
@ajuszczak ajuszczak changed the title Bitcoin docker env Change the mechanism of reading configuration as well as the style of it Jul 8, 2020
docker/entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@molowny molowny left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ivanopagano ivanopagano left a comment

Choose a reason for hiding this comment

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

Waiting for your feedback on open comments

Copy link
Contributor

@ivanopagano ivanopagano 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 do it

@vishakh vishakh merged commit 4f2319e into master Jul 8, 2020
@ivanopagano ivanopagano added the configuration-change-required There are adjustments needed to properly deploy the application after the code is accepted label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Btc-Int Bitcoin Integration configuration-change-required There are adjustments needed to properly deploy the application after the code is accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants