Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-598. Support get environment list from database #374

Closed
wants to merge 10 commits into from

Conversation

kobe860219
Copy link
Contributor

@kobe860219 kobe860219 commented Aug 11, 2020

What is this PR for?

Improve environment swagger api.
When cache is null. Will get environment data from database.

What type of PR is it?

[Improvement]

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-598

How should this be tested?

https://travis-ci.org/github/kobe860219/submarine/builds/716904008?utm_source=github_status&utm_medium=notification

Screenshots (if appropriate)

截圖 2020-08-16 上午12 11 07

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@kobe860219 kobe860219 changed the title Submarine-598. Support get environment list from database SUBMARINE-598. Support get environment list from database Aug 11, 2020
@kobe860219
Copy link
Contributor Author

@liuxunorg @pingsutw @JohnTing Please help me review this pr. Thanks :)

Copy link
Member

@jiwq jiwq left a comment

Choose a reason for hiding this comment

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

Thanks @kobe860219 for the contribution.

return environmentList;

if (envs.size() != 0) {
return envs;
Copy link
Member

Choose a reason for hiding this comment

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

If direct return envs, Will this cause envs variable to never update?

@kobe860219
Copy link
Contributor Author

kobe860219 commented Aug 12, 2020

@jiwq @liuxunorg @pingsutw @JohnTing I updated this pr and screenshots. I add a path /list that support get environments data from database by /v1/environment/list . It is able to support environment page of workbench show environments list from database.

@kobe860219 kobe860219 requested a review from xunliu August 12, 2020 14:19
content = @Content(
schema = @Schema(
implementation = Environment.class)))})
public Response listAllEnvironments() {
Copy link
Member

Choose a reason for hiding this comment

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

There is a listEnvironment in EnvironmentRestApi.java, why create a new function listAllEnvironments?

public Response listEnvironment(@QueryParam("status") String status) {
try {
List<Environment> environmentList =
environmentManager.listEnvironments(status);
return new JsonResponse.Builder<List<Environment>>(Response.Status.OK)
.success(true).result(environmentList).build();
} catch (SubmarineRuntimeException e) {
return parseEnvironmentServiceException(e);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

listEnvironment just get those environments in cache. listAllEnvironments will get environments from database.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can merge it rather than create new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will merge it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kobe860219 Thanks for working on this. Even from API end point perspective, I don't see a need for "list". Just typing "/v1/environment" should return the list of environments. Change is required only in model layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiwq @pingsutw @manirajv06 I have updated this pr. Please take a look, Thanks :))

@jiwq
Copy link
Member

jiwq commented Aug 17, 2020

@kobe860219 Could you fix the failed tests: EnvironmentRestApiTest.listEnvironment:129 expected:<1> but was:<2>? https://travis-ci.org/github/apache/submarine/jobs/718209689

@kobe860219
Copy link
Contributor Author

@jiwq I have fixed the failed tests :)

@jiwq
Copy link
Member

jiwq commented Aug 18, 2020

+1 Will commit if no more comments.

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

LGTM

@asfgit asfgit closed this in 689ae3c Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants