Grafana api #86
Grafana api #86
Conversation
| public GrafanaClient(WSClient ws, @Named("grafana") Configuration grafanaConf) { | ||
| this.ws = ws; | ||
|
|
||
| this.apiKey = grafanaConf.getString("api_key"); |
There was a problem hiding this comment.
So are we going to use the api_key at the end?
There was a problem hiding this comment.
I would like to, but there is something wrong in grafana that does not let me.
I will open a ticket and fix that later.
| String scheme = grafanaConf.getString("scheme"); | ||
| String host = grafanaConf.getString("host"); | ||
| String port = grafanaConf.getString("port"); | ||
| this.baseUrl = scheme + "://" + host + ":" + port; |
There was a problem hiding this comment.
Java has a URL class you can use if I'm not wrong.
There was a problem hiding this comment.
Yes but the Play client only accept String, I don't know if it's worth the double transformation, String -> URL -> String
There was a problem hiding this comment.
You can pass from string to URL and URL to string. The only benefit you are going to have is validating the URL in construction, but it's not so important here :)
|
|
||
| return getWsRequestForUserCreation(adminUserEndpoint).post(userRequest).thenApply(response -> { | ||
| Logger.debug(response.getBody()); | ||
| if (response.getStatus() == Http.Status.OK) { |
There was a problem hiding this comment.
Don't we need to do something if the request fails?
There was a problem hiding this comment.
sound good move this ok to a method no?
There was a problem hiding this comment.
@pedrovgs I created a ticket #88, for the MVP I don't want to handle the failure of grafana api.
@flipper83 👍
| } | ||
|
|
||
| public CompletionStage<GrafanaResponse> deleteUserInDefaultOrganisation(User user) { | ||
| String adminUserEndpoint = "/api/orgs/:orgId/users/:userId".replaceFirst(":orgId", "1").replaceFirst(":userId", user.getGrafanaUserId()); |
There was a problem hiding this comment.
As this string is repeated in some methods we could extract it into a constant.
|
|
||
| ObjectNode userRequest = Json.newObject() | ||
| .put("loginOrEmail", user.getEmail()) | ||
| .put("role", "Viewer"); |
There was a problem hiding this comment.
If you don't want to create json manually you can create a class as you tend to do with the responses :)
There was a problem hiding this comment.
I no strong opinion about that, coming from swift, this seems fine, until I need this model more than once.
| } | ||
|
|
||
| @NotNull | ||
| private String generatePassword() { |
There was a problem hiding this comment.
Nice password generator!! I'd move this method to a PasswordGeneratorclass just to don't mix responsibilities between this api client and the pass generator :)
| } | ||
|
|
||
| private void addUserToGrafanaOrg(User user) { | ||
| String[] split = user.getEmail().split("@"); |
There was a problem hiding this comment.
If our user has a gmail email is going to share the org with other gmail users?
There was a problem hiding this comment.
No, domain should be set in Org configuration.
We will only allow domain, that are not @gmail.com or @googlemail.com
There was a problem hiding this comment.
That's for a future PR.
There was a problem hiding this comment.
ok, but if we only allow domains that are not @gmail.com how is a regular user w//o working at any organization going to test the platform?
| @@ -27,5 +27,10 @@ protected void configure() { | |||
| bind(MetricsDatasource.class) | |||
| .to(ElasticSearchDatasource.class) | |||
| .asEagerSingleton(); | |||
|
|
|||
| Configuration grafanaConf = configuration.getConfig("grafana"); | |||
There was a problem hiding this comment.
have sense move this grafana literal to a constant?
There was a problem hiding this comment.
I will do it, it feels like a java thing to me :D
have sense
| this.baseUrl = scheme + "://" + host + ":" + port; | ||
| } | ||
|
|
||
| public CompletionStage<GrafanaResponse> createUser(final User user) { |
There was a problem hiding this comment.
are you using any kind of checkstyle? do you have a typo here, a double space between public and return value.
There was a problem hiding this comment.
There is no checkstyle in this project I will add one later.
There was a problem hiding this comment.
try to add as soon as possible because fix check style after having a lot of code could be a pain in the ass. @pedrovgs or I can help you with this one.
|
|
||
| String grafanaPassword = generatePassword(); | ||
| ObjectNode userRequest = Json.newObject() | ||
| .put("name", user.getName()) |
There was a problem hiding this comment.
have sense move this request properties to constants?
|
|
||
| return getWsRequestForUserCreation(adminUserEndpoint).post(userRequest).thenApply(response -> { | ||
| Logger.debug(response.getBody()); | ||
| if (response.getStatus() == Http.Status.OK) { |
There was a problem hiding this comment.
sound good move this ok to a method no?
| } | ||
|
|
||
| public CompletionStage<GrafanaResponse> addUserToOrganisation(User user, Organization organization) { | ||
| String adminUserEndpoint = "/api/orgs/:orgId/users".replaceFirst(":orgId", organization.grafanaId); |
There was a problem hiding this comment.
can you move those urls to a constants?
|
|
||
| private void addUserToGrafanaOrg(User user) { | ||
| String[] split = user.getEmail().split("@"); | ||
| if (split.length == 2) { |
There was a problem hiding this comment.
can you give semantic to this 2 with a constant?
📌 References
🎩 What is the goal?
When user login for the first time it create his user in grafana in addition of the actual flowup db.
Also if his orgs is configure with a google domain, it will automatically join the org.
How is it being implemented?
Using the Grafana API and Play authenticate callback