Skip to content
This repository has been archived by the owner on May 18, 2020. It is now read-only.

Replaced the underlying Cassandra image #19

Merged
merged 1 commit into from
Jul 21, 2017
Merged

Replaced the underlying Cassandra image #19

merged 1 commit into from
Jul 21, 2017

Conversation

jpkrohling
Copy link
Collaborator

@jpkrohling jpkrohling commented Jul 19, 2017 •
edited by pavolloffay
Loading

Fixes #11

@jpkrohling
Copy link
Collaborator Author

Blocked by: uber/jaeger#278 . This won't work until the image jaegertracing/jaeger-cassandra-schema is published with those changes.

@pavolloffay
Copy link
Member

@jpkrohling could you please add a test which would ping /rest/dependencies to the BaseTest? Currently dep. endpoint returns an error due to incompatible C* version and this PR fixes it. It resolves #16

@jpkrohling
Copy link
Collaborator Author

The test was there already (thanks!), I just needed to remove the override with the @Ignore. It's still failing because the create-schema image isn't available yet.

pavolloffay
pavolloffay reviewed Jul 21, 2017
View reviewed changes
production/src/test/java/io/jaegertracing/kubernetes/ProductionETest.java Outdated

/**
* @author Pavol Loffay
*/
public class ProductionETest extends BaseETest {

@Ignore("dependency links returns 404 because of old Cassandra image")
Copy link
Member

Choose a reason for hiding this comment

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

This test will still fail even with fixed c*.

I would like to see added a test which would ping dependencies REST endpoint.

@jpkrohling
Copy link
Collaborator Author

Tests are now passing locally:

[INFO] Reactor Summary:
[INFO] 
[INFO] jaegertracing-kubernetes-parent .................... SUCCESS [  1.468 s]
[INFO] jaegertracing-kubernetes-deployment-itest .......... SUCCESS [  2.478 s]
[INFO] jaegertracing-kubernetes-all-in-one ................ SUCCESS [ 18.992 s]
[INFO] jaegertracing-kubernetes-production ................ SUCCESS [02:47 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:10 min
[INFO] Finished at: 2017-07-21T10:24:00+02:00
[INFO] Final Memory: 30M/311M

@jpkrohling
Copy link
Collaborator Author

@pavolloffay I think this is now ready to be merged.

pavolloffay
pavolloffay suggested changes Jul 21, 2017
View reviewed changes
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Why using jaeger tag 0.5? If I would prefer to stick to our convention and keep latest and use tagged images only when we are going to tag the templates.

production/src/test/java/io/jaegertracing/kubernetes/ProductionETest.java Outdated
@@ -14,15 +14,16 @@
package io.jaegertracing.kubernetes;
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be change in this file

pavolloffay
pavolloffay reviewed Jul 21, 2017
View reviewed changes
...ernetes-deployment-itest/src/main/java/io/jaegertracing/kubernetes/deployment/BaseETest.java Outdated
@@ -128,6 +134,14 @@ public void testDependencyLinks() throws IOException, InterruptedException {
}
}

@Test
public void hitDependencyScreen() throws IOException {
InputStream response = new URL(queryUrl + "/api/dependencies?endTs=0").openStream();
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the same http client as in other tests? There is probably missing an assert on the status code.

@jpkrohling jpkrohling merged commit e9aabb9 into jaegertracing:master Jul 21, 2017
@pavolloffay pavolloffay mentioned this pull request Jul 27, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jpkrohling @pavolloffay