add get-container functionality #241

kidsbear · 2017-07-20T21:20:03Z

For issue #236

ratulm · 2017-07-20T21:25:22Z

Hey, Wei - I am curious if this functionality is not something that you could have provided through the existing listContainers() API call?

kidsbear · 2017-07-20T21:41:39Z

Actually, I am not sure about this. The existing listContainers() API call requires the api key has the authority to list all containers under the folder, I am not sure if we also need the authority for get-container call. Also, if we only want to see if the container exist, we can check if the container is in the return list, I am not sure whether we need more information(testrig, etc).

ratulm · 2017-07-21T00:49:12Z

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgrService.java

+   @POST
+   @Path(CoordConsts.SVC_RSC_GET_CONTAINER)
+   @Produces(MediaType.APPLICATION_JSON)
+   public JSONArray getContainer(


getContainer() is perhaps not the right name for this function. Perhaps:

getContainerInfo() if the plan down the line is to provide more information about the container (e.g., time of creation)

doesContainerExists() is the intent is to simply let the clients check if the container with the provided name exists. (that is the purpose this function is currently serving, as no other information is provided.)

getting back to your response, @kidsbear, i see the difference between the two functions. yes, being able to check if a containerName exists without having access to it is useful (and not supported by listContainers).

and by that token, the purpose of this function should stay as something close to doesContainerExist() as no other info should be provided with clients with API keys that don't have access to the container.

Now the command used is 'exist-container', will output a string whether indicates the container does not exist, or the creation time of the container

… issue236-container merge branch

ratulm · 2017-07-21T01:38:38Z

Sorry, I may have been unclear, but this behavior is unclear and not intuitive.

Suggestions:

instead of existContainer, call things checkContainerExists
return just true or false accordingly

Makes sense?

kidsbear · 2017-07-21T01:43:33Z

Sure, will fix it later:)

dhalperi · 2017-07-21T04:55:06Z

I don't buy it.i think getContainer is the right RESTful name for the function. -- GET on a particular container to get its information. Let's discuss tomorrow

…

On Thu, Jul 20, 2017 at 18:43 kidsbear ***@***.***> wrote: Sure, will fix it later:) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#241 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgIT6nXKlo09ot0ZH0F-N8lJ8wm__bWks5sQAJGgaJpZM4Oerx1> .

ratulm · 2017-07-25T05:30:05Z

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java

+      SortedSet<String> testrigs = new TreeSet<>(
+            CommonUtil.getSubdirectories(containerDir).stream()
+                  .map(dir -> dir.getFileName().toString())
+                  .collect(Collectors.toSet()));


Technically, testrigs do not sit directly under the container directory. They sit under /BfConsts.RELPATH_TESTRIGS_DIR. (The constant is "" right now for backward compatibility but we'll make it something else at some point.)

Or, perhaps, your intent was to show all subdirectories under containerDir, and I am confused by the variable names saying 'testrigs' in some places.

ratulm · 2017-07-25T05:31:30Z

projects/batfish-client/src/main/java/org/batfish/client/BfCoordWorkHelper.java

+         }
+
+         String testrigs = response.readEntity(String.class) + "\n";
+         return testrigs;


Consistent with server-side code, use 'containerInfo' as variable name instead of 'testrigs'?

…e236-container merge branch

dhalperi · 2017-07-26T04:28:07Z

projects/batfish-client/src/main/java/org/batfish/client/BfCoordWorkHelper.java

@@ -385,6 +385,52 @@ private ClientBuilder getClientBuilder() throws Exception {
        .register(MultiPartFeature.class);
  }

+  /**
+   * Return true if successfully get the information of the container,


This does not seem up to date.

dhalperi · 2017-07-26T04:29:36Z

projects/batfish-client/src/main/java/org/batfish/client/Client.java

@@ -1322,6 +1322,23 @@ private boolean getAnswer(
    return parameters;
  }

+  /**
+   * Get information of the container(first element in {@code parameters}) return true if


Please clean up this javadoc. Space before ( and I can't tell if this is 1 or 2 sentences, etc. Use punctuation.

dhalperi · 2017-07-26T04:30:06Z

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java

@@ -531,6 +531,20 @@ public String getAnswer(
    return answer;
  }

+  /**
+   * Return a {@link SortedSet SortedSet} contains all subdirectories under the path {@code


A Container is not a sorted set -- it should be a Container. Please make a proper java object that represents a container. One field it may have is testrigs, which could be an ordered list or set of testrigs.

dhalperi · 2017-07-26T04:34:08Z

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgrService.java

+      checkContainerAccessibility(apiKey, containerName);
+
+      SortedSet<String> containerInfo = Main.getWorkMgr().getContainer(containerDir);
+      return Response.ok(containerInfo.toString(), MediaType.TEXT_PLAIN).build();


This should be a JSON response of a container.

dhalperi · 2017-07-26T04:38:59Z

projects/coordinator/src/test/java/org/batfish/coordinator/WorkMgrTest.java

+    _folder.newFolder(containerName);
+    Main.mainInit(new String[] {"-containerslocation", _folder.getRoot().toString()});
+    Path containerDir = Paths.get(_folder.getRoot().toPath().resolve(containerName).toString());
+    SortedSet<String> containerInfo = _manager.getContainer(containerDir);


Most of the lines in these tests are identical. Can you pull all the setup of the Main and the containers to a helper function and simplify the tests dramatically?

dhalperi · 2017-07-26T04:40:00Z

projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgrService.java

+  @POST
+  @Path(CoordConsts.SVC_RSC_GET_CONTAINER)
+  @Produces(MediaType.TEXT_PLAIN)
+  public Response getContainer(


Where is the test of this function?

dhalperi

Minor changes requested, but looks great!

dhalperi · 2017-07-26T23:51:27Z

projects/batfish-common-protocol/src/main/java/org/batfish/common/Container.java

+    _analysis = analysis;
+  }
+
+  public String listTestrigs() {


drop this function – it's just a thin wrapper around getTestrigs, isn't it?

dhalperi · 2017-07-26T23:52:50Z

projects/batfish-client/src/main/java/org/batfish/client/Client.java

+  /**
+   * Get information of the container(first element in {@code parameters}).
+   * <p> </p>
+   * Returns true if successfully get container information, false otherwise


Get information of the container (first element in {@code parameters}). <p>Returns {@code true} if successfully get container information, {@code false} otherwise.

…e236-container merge branch

… issue236-container merge

add get-container functionality

8982946

kidsbear requested a review from dhalperi July 20, 2017 21:20

Merge branch 'master' into issue236-container

9ed1b22

ratulm reviewed Jul 21, 2017

View reviewed changes

kidsbear added 5 commits July 20, 2017 18:28

only check if the container exist, output creation time if exist

e812cf3

Merge branch 'issue236-container' of github.com:kidsbear/batfish into…

ce89fd5

… issue236-container merge branch

Merge branch 'master' into issue236-container

37cf192

rename constant variable

632ea42

Merge branch 'issue236-container' of github.com:kidsbear/batfish into…

63dea36

… issue236-container merge branch

kidsbear added 2 commits July 23, 2017 18:00

merge branch

d2abbd5

implement get-container

b47b239

ratulm reviewed Jul 25, 2017

View reviewed changes

kidsbear added 4 commits July 25, 2017 09:00

Merge branch 'master' of https://github.com/batfish/batfish into issu…

6453b0e

…e236-container merge branch

modify variable name, enforce a consistent name for both sides

cd35e32

reformat modified code

dfba6af

resolve conflicts

d8ab4aa

dhalperi requested changes Jul 26, 2017

View reviewed changes

kidsbear added 2 commits July 26, 2017 16:20

create Container object to represent a container

b888c68

avoid creating duplicate temp files

f9fe6d6

dhalperi requested changes Jul 26, 2017

View reviewed changes

kidsbear and others added 4 commits July 26, 2017 17:05

remove unneccessary method

50b1501

Merge branch 'master' into issue236-container

7d01c14

Merge branch 'master' into issue236-container

c0a9840

Merge branch 'master' of https://github.com/batfish/batfish into issu…

f85eed4

…e236-container merge branch

Merge branch 'issue236-container' of github.com:kidsbear/batfish into…

dbdf115

… issue236-container merge

dhalperi approved these changes Jul 27, 2017

View reviewed changes

dhalperi merged commit 5af643f into batfish:master Jul 27, 2017

dhalperi mentioned this pull request Aug 1, 2017

Coordinator should have a way to get info about a container #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add get-container functionality #241

add get-container functionality #241

kidsbear commented Jul 20, 2017

ratulm commented Jul 20, 2017

kidsbear commented Jul 20, 2017

ratulm Jul 21, 2017

ratulm Jul 21, 2017

kidsbear Jul 21, 2017

ratulm commented Jul 21, 2017

kidsbear commented Jul 21, 2017

dhalperi commented Jul 21, 2017 via email

ratulm Jul 25, 2017 •

edited

ratulm Jul 25, 2017

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

dhalperi left a comment

dhalperi Jul 26, 2017

dhalperi Jul 26, 2017

add get-container functionality #241

add get-container functionality #241

Conversation

kidsbear commented Jul 20, 2017

ratulm commented Jul 20, 2017

kidsbear commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratulm commented Jul 21, 2017

kidsbear commented Jul 21, 2017

dhalperi commented Jul 21, 2017 via email

ratulm Jul 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhalperi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratulm Jul 25, 2017 •

edited