-
Notifications
You must be signed in to change notification settings - Fork 476
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
fix #365 - fix problem of connecting to the influx api with URL which… #400
Conversation
… URL which does not points to the url root (e.g. localhots:80/influx-api/) This change was tested with following URLs: http://localhost:8086 http://localhost:8086/ Proxied connection (influx behind Nginx) http://loocalhost:8080/influx-api/
Codecov Report
@@ Coverage Diff @@
## master #400 +/- ##
============================================
+ Coverage 86.55% 86.63% +0.07%
- Complexity 301 302 +1
============================================
Files 20 20
Lines 1294 1294
Branches 135 135
============================================
+ Hits 1120 1121 +1
+ Misses 114 113 -1
Partials 60 60
Continue to review full report at Codecov.
|
@vicctor would be great if you could provide a docker-compose.yml file containing both nginx properly configured and influxdb running so I could run the tests against it. |
…ss different URL's for testing (also remoted unused code with selection of API notAPI port)
Anyway, I believe that it's better to keep the URL in single URL property
- changed compile-and-test.sh scripts, so it starts both standard and "proxy" - added simple nginx config for docker container - tests related to UDP communication moved to separate class and skipped in case of http proxy testing
Hi @vicctor Please do not format any of the files for a PR, try to reduce the changes needed, you have actually add more than 1000 lines and remove about 800, this makes it impossible to review this PR. |
@majst01 Hi Steffan, thx for quick response. The truth is that I'm new in this project and I was in hurry to fix this issue, so all I can say sorry about that. Also small explanation about what I did: |
thanks for ur contributions |
@vicctor if you still interested in this PR you need to rebase or resolve the conflicts. |
This PR does basically the same as #416 |
pom.xml
Outdated
@@ -89,6 +89,11 @@ | |||
<groupId>org.apache.maven.plugins</groupId> | |||
<artifactId>maven-surefire-plugin</artifactId> | |||
<version>2.21.0</version> | |||
<configuration> | |||
<excludes> | |||
<exclude>${someModule.test.excludes}</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this meant for
import org.influxdb.dto.Pong; | ||
import org.junit.jupiter.api.Assertions; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.platform.runner.JUnitPlatform; | ||
import org.junit.runner.RunWith; | ||
|
||
import okhttp3.OkHttpClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why reformatting this import
src/test/nginx/nginx.conf
Outdated
root /usr/share/nginx/html; | ||
} | ||
|
||
# proxy the PHP scripts to Apache listening on 127.0.0.1:80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of unrelated and commented code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2c84650
to
511ab70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there
@RunWith(JUnitPlatform.class) | ||
public class InfluxDBProxyTest { | ||
private InfluxDB influxDB; | ||
private String db = "udp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database should be named via-proxy or so, but not udp, this is misleading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change naming
BTW, name of the database for UDP is configured at server side
} | ||
|
||
public static String getProxyApiUrl() { | ||
return getEnv("PROXY_API_URL", "http://127.0.0.1:8086/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please check formatting, here identation is broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it, we should have check style for test as well
+ code format + test db naming
… does not points to the url root (e.g. localhots:80/influx-api/)
This change was tested with following URLs:
http://localhost:8086
http://localhost:8086/
Proxied connection (influx behind Nginx)
http://loocalhost:8080/influx-api/