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

Service labels #10

Merged
merged 8 commits into from
May 3, 2017
Merged

Service labels #10

merged 8 commits into from
May 3, 2017

Conversation

tegataiprime
Copy link
Contributor

Adjusted MarathonDiscoveryClient to return Marathon Application Labels (a map of key/value pairs) as the Spring Cloud Discovery ServiceInstance meta-data (also a map of key/value pairs). This will facilitate applying Predicate Filters using Ribbon e.g. "fetch all service instances with the following key/value combinations". This enables us to also announce service capabilities.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 66.667% when pulling 1525404 on tegataiprime:serviceLabels into 7cfc277 on aatarasoff:master.

@aatarasoff
Copy link
Owner

aatarasoff commented Apr 5, 2017

Nice feature.
Would you like to provide possible usage in example project and documentation?

@tegataiprime
Copy link
Contributor Author

Yes, of course. I am working on another complimentary feature to locate services using a regular expression or wild card. Then applying a ribbon filter over ServiceInstance meta-data makes a lot more sense, as each service located could have a different set of labels. This kind of thing really helps when applying rolling updates. I'll keep working on this branch & will update the documentation & example application.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 66.667% when pulling a531009 on tegataiprime:serviceLabels into 7cfc277 on aatarasoff:master.

…ervice labels. Note: depends on 0.3.1-SNAPSHOT of the Marathon Client (to support secrets returned in App.env)
@tegataiprime
Copy link
Contributor Author

Note: depends upon com.mesosphere:marathon-client:0.3.1-SNAPSHOT (to support DC/OS secrets that are returned in App.env)

GetAppsResponse getApps() throws MarathonException;

@RequestLine("GET /v2/apps")
GetAppsResponse getApps(@QueryMap Map<String, String> queryMap) throws MarathonException;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new method to support passing query parameters to /v2/apps
e.g.
/v2/apps?id=myServiceName

Submitted a PR to mesosphere/marathon-client for this change:
https://github.com/mesosphere/marathon-client/pull/44

@@ -22,6 +22,8 @@ allprojects {

repositories {
jcenter()
mavenLocal()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is temporary. Have built com.mesosphere:marathon-client:0.3.1-SNAPSHOT locally.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we could not wait until new Marathon lib version will be released, cause you've already patched Marathon client. So, remove that for passing travis tests and I merge your PR.

@@ -40,7 +42,7 @@ allprojects {

dependencies {
dependency 'org.projectlombok:lombok:1.16.8'
dependency 'com.mesosphere:marathon-client:0.3.0'
dependency 'com.mesosphere:marathon-client:0.3.1-SNAPSHOT'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using com.mesosphere:marathon-client:0.3.1-SNAPSHOT (built locally) to support DC/OS secrets in App.env

Copy link
Owner

Choose a reason for hiding this comment

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

See my another comment about maven local.

@aatarasoff aatarasoff merged commit 313cde2 into aatarasoff:master May 3, 2017
@aatarasoff
Copy link
Owner

There are some troubles with PR after merging.
Maybe you not commit some files like DCOSClient and etc?

Could you fix the problem?

@aatarasoff
Copy link
Owner

Anyway, I've eliminated DCOS part, reverted marathon client lib version to 0.3.0 and fixed build.

Thank for you pull request. If you want DCOS part to comeback, please, merge with master and make new pull request with changes that will pass CI build.

@aatarasoff
Copy link
Owner

Do you have twitter? I wanna mention you in tweet about new version of starter.

@tegataiprime
Copy link
Contributor Author

Yes. My twitter handle is @tegatai

@tegataiprime
Copy link
Contributor Author

I'm talking with the chaps at Mesosphere about getting my PR for QueryParms merged in and to have them release their 0.3.1 version. We have a commercial relationship with them, so I am hopeful this will happen soon.

When that is done I'll be able to re-commit the changes for DCOS authentication. In the meantime, I've created my own artifacts for marathon-client:0.3.1 & spring-cloud-marathon (with DCOS authentication) to keep our internal development moving forward.

@tegataiprime tegataiprime deleted the serviceLabels branch May 3, 2017 19:30
@aatarasoff
Copy link
Owner

Ok, that's great!

@tegataiprime
Copy link
Contributor Author

My changes to marathon-client have been accepted & merged into their master. Waiting on Mesosphere for a release of 0.3.1.

@tegataiprime
Copy link
Contributor Author

Mesosphere are preparing to release marathon-client 0.5.0, which contains my changes. When it hits maven central I'll open a PR to merge in support for DCOS authentication.

@aatarasoff
Copy link
Owner

Ok, it is great news. Will be waiting for changes)

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

3 participants