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

refactor: adjust configuration management and smoke test setup to support production config by default #232

Conversation

ata-nas
Copy link
Contributor

@ata-nas ata-nas commented Oct 4, 2024

Description:

  • make the default values in config records to be the production ones;
  • the app.properties file should not be setting the production values, by default it should be empty, we should only set props there if we want to override the production defaults;
  • when executing smoke tests, the config records should be with test values;
  • explore options to achieve the result: (1) use env variables - does not seem to work as expected, the config dependency gets the vars via System.getenv() and it only shows case-sensitive keys, so adding a value like mediator.ringBufferSize=1024 will result into not being visible in runtime, it seems to me to be inconsistent. (2) build the jar for the smoke tests with the app.properties from src/test/resources - this works, the file gets packaged into the jar, only hurdle currently is to find a good and reliable way to do it with a gradle task.
  • a single gradle task execution should be sufficient to build and run the needed environment for the test to pass

UPDATE:

  • after some discussions with @mattp-swirldslabs & @AlfredoG87, we have discussed some issues that we have discovered:

    1. environment variables that are set to the docker container are read only if they are all upper case and separated only via _.;
    2. the initial idea of making a single gradle task (see some previous commits in this PR) that will build the project with correct test props, create the docker image and run it (so we can have an easy single entry point) did not seem to work when I tried it, since when in gh actions I execute either a shell script or the gradle task that contains a shell invocation of the docker executable, gh actions failed with exit code 14 (insufficient privileges). After some research, I see that I probably need to use the docker executable directly in the run part of the step. That worked and the test passed after commiting. So this part is still debatable, can I or can I not use the docker executable from a shell script.;
  • since this discussions, there are several discoveries:

    1. using the IntelliJ IDE, if we modify the run config to include an environment variable with lowercase and . separator, it gets discovered. I am still to understand how that works and why, it does not seem to get passed as an CLI arg when starting the jar, so the IDE is doing something else. But that only means that it is indeed possible to discover non-conventional keys for environment variables.;
    2. as an approach, we could use PropertyFileConfigSource.class and read vars directly from a file (this is different from ClasspathFileConfigSource.java where we technically still load a file app.properties but this is packaged into the jar, here we are talking about an arbitrary file outside the classpath), this worked and is a potential approach. However, it has the lowest precedence, so if the same key comes from basically any other source, this will get overridden, we need to keep that in mind. Ordinals could be set in the constructor, so we can decide precedence. (see ConfigSourceOrdinalConstants.java);
  • final approach (or the usage of multiple approaches so more could be tested if decided so) is still under debate;

UPDATE:

  • after a discussion with @jsync-swirlds, he noted that we will be able to support by default unconventional casing for environment variables (meaning an environment variable like mediator.ringBufferSize=1024 will be picked up without having to do anything special). Until then, we can use the MappedConfigSource in order to support the mapping between uppercase variables and the formate we need them (for example the env variable MEDIATOR_RING_BUFFER_SIZE=1024 will be available with that key as well as with this one mediator.ringBufferSize=1024). For now, we can only pick up uppercase variables with an _ separator, but with the mapping, we will be able to read the values with the mapped key.

Related issue(s):

Fixes #185

Notes for reviewer:

Tested locally:

  • Start the jar directly and use app.properties
  • Start the jar directly and use env vars
  • Start the docker container and use app.properties
  • Start the docker container and use env vars
  • Start the smoke-test action locally and use app.properties
  • Start the smoke-test action locally and use env vars
  • Start k8s cluster with the container and use app.properties
  • Start k8s cluster with the container and use env vars
  • Started all of the above with both app.properties and env vars, expected that the env vars take precedence (as is by default and we have not changed the default precedence)

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@ata-nas ata-nas added the Improvement Code changes driven by non business requirements label Oct 4, 2024
@ata-nas ata-nas added this to the 0.1.0 milestone Oct 4, 2024
@ata-nas ata-nas self-assigned this Oct 4, 2024
@ata-nas ata-nas changed the title refactor: adding some context and comments, still exploring locally the best approach refactor: adjust configuration management and smoke test setup to support production config by default Oct 4, 2024
suites/build.gradle.kts Outdated Show resolved Hide resolved
@ata-nas ata-nas marked this pull request as ready for review October 10, 2024 11:35
@ata-nas ata-nas requested review from a team as code owners October 10, 2024 11:35
@ata-nas ata-nas marked this pull request as draft October 10, 2024 11:37
Copy link
Contributor

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

I know it's still on draft, but so far it looks good and I don't have any questions or nits 👍

@AlfredoG87 AlfredoG87 modified the milestones: 0.1.0, 0.2.0 Oct 12, 2024
@ata-nas ata-nas marked this pull request as ready for review October 14, 2024 07:39
georgi-l95
georgi-l95 previously approved these changes Oct 15, 2024
Copy link
Contributor

@georgi-l95 georgi-l95 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

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

I added a few comments

rbarkerSL
rbarkerSL previously approved these changes Oct 15, 2024
Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

Looks good, just one nitpick (which can be addressed later), and one block of text that should be a github discussion rather than a comment in code.

Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Looks good. Since you changed something in .github/workflows you might need approval from someone in RE or DevOps.

Copy link
Contributor

@AlfredoG87 AlfredoG87 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

@rbarkerSL rbarkerSL left a comment

Choose a reason for hiding this comment

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

Review applies to

  • .github/workflows/smoke-test.yaml
  • README.md

LGTM

@jsync-swirlds
Copy link
Member

@ata-nas, it appears that most of your commits on this PR are unsigned.
You can fix this with the following git commands:

git rebase --gpg-sign -f HEAD~20
git push --force

@ata-nas ata-nas force-pushed the 185-refactor-adjust-configuration-management-and-smoke-test-setup-to-support-production-config-by-default branch from 6dd22fe to 4f39560 Compare October 17, 2024 09:43
@ata-nas ata-nas merged commit c0ce015 into main Oct 17, 2024
13 checks passed
@ata-nas ata-nas deleted the 185-refactor-adjust-configuration-management-and-smoke-test-setup-to-support-production-config-by-default branch October 17, 2024 10:05
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@611e91b). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage        ?   99.78%           
  Complexity      ?      259           
=======================================
  Files           ?       52           
  Lines           ?      938           
  Branches        ?       64           
=======================================
  Hits            ?      936           
  Misses          ?        2           
  Partials        ?        0           
Files with missing lines Coverage Δ
...er/config/ServerMappedConfigSourceInitializer.java 100.00% <100.00%> (ø)
...m/hedera/block/server/mediator/MediatorConfig.java 100.00% <ø> (ø)
...m/hedera/block/server/notifier/NotifierConfig.java 100.00% <ø> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Code changes driven by non business requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: adjust configuration management and smoke test setup to support production config by default
6 participants