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

Adds configurable kv watch and listener #97

Closed
wants to merge 9 commits into from

Conversation

toast2e
Copy link

@toast2e toast2e commented Oct 13, 2015

Pulled fresh copy of project and added the kv watch on top. No more issues with 3 way merge. Also updated the event thrown to contain a full map of changed keys and values at the request of a user.

@@ -248,6 +248,11 @@
<version>${lombok.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the commons-collections dependency.

@spencergibb
Copy link
Member

Your build failed with

java.lang.NoClassDefFoundError: org/springframework/cloud/consul/config/watch/ConsulKeyValueChangeEvent
    at java.lang.Class.getDeclaredMethods0(Native Method)
    at java.lang.Class.privateGetDeclaredMethods(Class.java:2615)
    at java.lang.Class.getDeclaredMethods(Class.java:1860)
    at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:606)
    at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:518)
    at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:504)
    at 

any ideas?

@spencergibb
Copy link
Member

I think with a few tweaks this will be good.

@toast2e
Copy link
Author

toast2e commented Nov 18, 2015

No clue what happened with that build failure. That class should and appears to have been included in the initial commit.

@spencergibb
Copy link
Member

@toast2e as far as I can tell, this only updates existing properties? It doesn't do deletes or add new properties. Is that correct?

@toast2e
Copy link
Author

toast2e commented Nov 21, 2015

It should do all of those things. There are test included for each of those
scenarios (although I've been told by some users that deletes are a little
wonky which I need to look into).

@EnableConfigurationProperties
@EnableScheduling
@ConditionalOnProperty(name = "spring.cloud.consul.config.watch", matchIfMissing = false)
public class ConsulConfigWatchConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

This should be conditional on the actuator Endpoint.class.

Copy link
Author

Choose a reason for hiding this comment

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

In addition to the property or instead of? Also, not 100% sure I follow. Adding the following to the class:
@conditional(Endpoint.class)
Doesn't appear to work as the Endpoint class doesn't extend Condition. Is this what you meant or did I misunderstand?

Copy link
Member

Choose a reason for hiding this comment

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

@ConditionalOnClass(Endpoint.class)

Copy link
Member

Choose a reason for hiding this comment

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

In other words, this backs of if spring-boot-actuator isn't on the classpath.

@spencergibb
Copy link
Member

Sorry this hasn't gotten much attention over the holidays.

@toast2e
Copy link
Author

toast2e commented Jan 14, 2016

No worries. I was able to get in a better handling of deletes (worked fine for properties that existed at startup but had issues with ones that had been added later). Let me know if there's anything else that needs to be addressed.

@spencergibb
Copy link
Member

@toast2e you've got bunch of commits that aren't yours.

@spencergibb
Copy link
Member

I want to move on this before we do a release candidate.

@toast2e
Copy link
Author

toast2e commented Feb 2, 2016

I hope that did it. Pulled down latest, rebased, and force pushed everything back. Let me know if there are other issues.

@spencergibb
Copy link
Member

That looks much better.

@toast2e
Copy link
Author

toast2e commented Feb 2, 2016

And, of course after forcing an update of spring-cloud-commons, the updates aren't working again. This is going to need another round of tweaking.

@spencergibb
Copy link
Member

Closed in favor of d1141a8

@spencergibb spencergibb closed this Feb 8, 2016
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.

None yet

2 participants