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

PS-827: Enable harvester to use dynamic prefix for keys #103

Conversation

wimspaargaren
Copy link
Contributor

Which problem is this PR solving?

This PR addresses the following two issues:

  • Enable users to dynamically specify the consul location where a certain key is stored, since Go tags can't be dynamically adjusted
  • Reduce redundancy in consul tags

Short description of the changes

A new method is added to the harvester builder, called WithConsulFolderPrefixMonitor. This method allows you to specify an optional prefix of where harvester needs to look for your key in Consul.

This enables users to dynamically adjust the location of where a key is located in Consul. For example, if you deploy a service in multiple clusters, you might want to define the key location based on the deployment using an environment variable. In addition, this removes the redundancy for prefixes of keys.

Old way:

	Key1 sync.String `seed:"dummy value" consul:"services/my-service/key1"`
        Key2 sync.String `seed:"dummy value" consul:"services/my-service/key2"`
        Key3 sync.String `seed:"dummy value" consul:"services/my-service/key3"`

      .......

     WithConsulMonitor("consul-addr", "data-center", "token", time.Second * 42).

New way:

	Key1 sync.String `seed:"dummy value" consul:"key1"`
        Key2 sync.String `seed:"dummy value" consul:"key2"`
        Key3 sync.String `seed:"dummy value" consul:"key3"`

      .......

     WithConsulFolderPrefixMonitor("consul-addr", "data-center", "token", "services/my-service", time.Second * 42).

@wimspaargaren wimspaargaren force-pushed the PS-827-enable-harvester-to-use-dynamic-prefix-for-keys branch from 3b9b552 to 75a5de2 Compare October 8, 2021 09:33
Copy link
Contributor

@mantzas mantzas left a comment

Choose a reason for hiding this comment

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

LGTM

@mantzas mantzas merged commit 84469b7 into beatlabs:master Oct 8, 2021
@c0nstantx
Copy link
Contributor

@wimspaargaren It would be nice if you could update the examples and README with the new Consul setup.

cc @mantzas @Lazaros-Tsigkakos

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.

4 participants