Implemented more API complete container listing #315

milux · 2020-02-19T11:57:59Z

This PR extends the container listing API documented in https://docs.docker.com/engine/api/v1.35/#operation/ContainerList.
All parameters except limit are now supported, especially size and filters.

coveralls · 2020-02-19T12:00:17Z

Pull Request Test Coverage Report for Build 585

46 of 46 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 90.964%

Totals
Change from base Build 575:	0.4%
Covered Lines:	906
Relevant Lines:	996

💛 - Coveralls

milux · 2020-02-19T12:04:58Z

@amihaiemil Is test coverage for contributed code a hard requirement?
I just fixed some tests to reflect the new code structure with ListedContainers.

0crat · 2020-02-19T12:10:32Z

This pull request #315 is assigned to @paulodamaso /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @amihaiemil /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job

paulodamaso

@milux I have some comments, please take a look.

paulodamaso · 2020-02-19T13:39:51Z

src/main/java/com/amihaiemil/docker/Containers.java

+     * @return Filtered containers.
+     * @see <a href="https://docs.docker.com/engine/api/v1.35/#operation/ContainerList">Docker API Docs</a>
+     */
+    Containers filter(Map<String, Iterable<String>> filters);


@milux I think that this should be implemented as a decorator called Filtered

Sorry, but I'm a bit puzzled because I did my implementation in line with the patterns I found in RtImages and ListedImages, and now you suggest a completely different pattern that has not been used in the library AFAICT.
Also, decorator patterns are applicable to one class only, because a decorator cannot expose functionality of multiple classes, right?
However, I think we could indeed improve code quality by defining this method in a separate interface (e.g. Filterable) and implementing that in all classes with that functionality.

@milux In this case we don't need essentially another interface; a Filtered would decorate a Containers object and execute the filtering of the values as needed. But we can develop a new Filterable interface, which will retain the filtering logic for all filterable elements (containers, images and so on). It is indeed a fine solution. @amihaiemil WDYT about this?

OK, I think we have to get some things straight here, because afaik the decorator pattern is an approach that simply doesn't make any sense here. It's almost like saying "let's use the observer pattern here". Please let me explain why I think so:
We could use the decorator pattern, whereas our decorator contains more or less precisely the same code as ListedContainers, plus a bunch of proxy methods that forward calls to the original RtContainer instance. We cannot remove all() and iterator(), because they need information about the filters and/or size flag to construct the API request, and the underlying implementation is per definition unaware of the wrapping decorator when using this pattern. Thus, we end up with a lot of code duplication, because we need to implement all() and iterator() twice, once for RtContainer and once for the decorator.
Maybe I'm an idiot and got your excellent idea all wrong, but I believe there might be an implicit error in your imagination about how this should work. If you still think decorators are a good idea, then please elaborate a bit more on this.

paulodamaso · 2020-02-19T13:43:26Z

src/main/java/com/amihaiemil/docker/Containers.java

+     * @param withSize Return the size of containers (SizeRw and SizeRootFs).
+     * @return Containers with modified size flag.
+     */
+    Containers withSize(final boolean withSize);


@milux I think that this could be implemented as a decorator to Containers, which returns decorated Container objects with size information

Same as above, maybe another interface?

@milux If we have the fields SizeRw and SizeRootFs for more API elements, then I think that an WithSize or Sized (I can't think in a good name for this) interface would be fine. But also, I think that we could create these objects inside a Containers decorator which returns only Sized objects. @amihaiemil how about?

And here is the decorator once again... see above. 😉

paulodamaso · 2020-02-19T13:46:18Z

src/main/java/com/amihaiemil/docker/ListedContainers.java

+ * @version $Id$
+ * @since 0.0.11
+ */
+final class ListedContainers extends RtContainers {


@milux Take a look to the comments above; after they are implemented this class may be not necessary and those behaviors can come back to RtContainers

Sure, we could move this functionality to RtContainers, but this is against the good pattern that I found with similar APIs in that library, where Sub-API-calls go to an abstract base class whereas concrete implementations for filter etc. go to a ListedXYZ class.
Do you really consider this a good idea?

@milux In this case we would have iterator and all methods moved to the abstract class. The other methods would be implemented by their corresponding interfaces

You're aware that interfaces do only define a contract/interface (thus their name...) and cannot contain any implementation?
Interface-like constructs that contain actual implementations, best known as traits, or other concepts for multiple inheritance are not available in Java.
We could move the project to Kotlin, though, then there are traits. 😉

And moving iterator and all to a base class that's not aware of filters and flags is not possible for the reasons explained above, unless you start to copy-paste the code to multiple levels of inheritance (or nesting in case of decorators) which is a terrible idea in terms of software design.

amihaiemil · 2020-02-19T15:04:08Z

@milux @paulodamaso Whatever the implementation details are, it doesn't matter as long as we can agree on the interfaces and class encapsulation.

Maybe @milux will provide more PRs, so we can adjust implementation details on the go.

@milux As guidelines, whenever you want to provide a PR, open an Issue about it first, so we can discuss there your ideas before you make a PR, it will be easier.

milux · 2020-02-19T16:01:19Z

@amihaiemil I totally agree, however, you may want to update your README to reflect this, because it now says If you would like to contribute, just open an issue or a PR., which AFAIK doesn't reflect this organizational philosophy. 😉

milux · 2020-02-19T16:01:59Z

What's missing for merge? Test coverage? Anything else?

paulodamaso

@milux @amihaiemil Please take a look at the comments

paulodamaso · 2020-02-19T16:23:54Z

src/main/java/com/amihaiemil/docker/Containers.java

+     * @return Filtered containers.
+     * @see <a href="https://docs.docker.com/engine/api/v1.35/#operation/ContainerList">Docker API Docs</a>
+     */
+    Containers filter(Map<String, Iterable<String>> filters);


@milux In this case we don't need essentially another interface; a Filtered would decorate a Containers object and execute the filtering of the values as needed. But we can develop a new Filterable interface, which will retain the filtering logic for all filterable elements (containers, images and so on). It is indeed a fine solution. @amihaiemil WDYT about this?

paulodamaso · 2020-02-19T16:27:49Z

src/main/java/com/amihaiemil/docker/Containers.java

+     * @param withSize Return the size of containers (SizeRw and SizeRootFs).
+     * @return Containers with modified size flag.
+     */
+    Containers withSize(final boolean withSize);


@milux If we have the fields SizeRw and SizeRootFs for more API elements, then I think that an WithSize or Sized (I can't think in a good name for this) interface would be fine. But also, I think that we could create these objects inside a Containers decorator which returns only Sized objects. @amihaiemil how about?

paulodamaso · 2020-02-19T16:51:37Z

src/main/java/com/amihaiemil/docker/ListedContainers.java

+ * @version $Id$
+ * @since 0.0.11
+ */
+final class ListedContainers extends RtContainers {


@milux In this case we would have iterator and all methods moved to the abstract class. The other methods would be implemented by their corresponding interfaces

amihaiemil · 2020-02-20T14:10:20Z

@milux @paulodamaso Guys, you seem to be having a long conversation I am honestly a bit lazy to follow. Just agree on the interface and details of implementation can be fixed later.

@milux About Decorator: we do use Decorator extensively. We believe Decorator is the most important pattern in OOP. We are actually practicing a more or less novel kind of OOP, called "Elegant Objects": https://www.elegantobjects.org/

I'm not sure what you guys are discussing about a Filtered pattern, but here is an example of such a Filtered class: https://amihaiemil.com/2018/11/10/an-extension-to-telldontask.html

Thanks,
Mihai

amihaiemil · 2020-02-20T14:11:41Z

@milux And anyway, I remember I have already implemented some Filtering in the library, somewhere. Is it not possible to follow that same pattern?

If you are following that same pattern, then it's ok :) I would just ask you to not decrease Test Coverage :D

milux · 2020-02-20T14:16:51Z

@milux And anyway, I remember I have already implemented some Filtering in the library, somewhere. Is it not possible to follow that same pattern?

If you are following that same pattern, then it's ok :) I would just ask you to not decrease Test Coverage :D

I am following the exact same pattern you have used in RtImages and ListedImages, it's not a decorator pattern, and I still believe that it's the best way how in can be done in terms of code design.

milux · 2020-02-20T14:18:46Z

@milux @amihaiemil Please take a look at the comments

Please see above, your comments somehow got duplicated as far as I can tell, and I commented above.

amihaiemil · 2020-02-20T14:24:23Z

@milux If it's the same pattern, it's fine. Just please don't decrease test coverage. Is it possible or too complicated? :D

@paulodamaso Let this one to me please :D

milux · 2020-02-20T14:31:41Z

@milux If it's the same pattern, it's fine. Just please don't decrease test coverage. Is it possible or too complicated?

Fun fact: It's exactly what you described in https://amihaiemil.com/2018/11/10/an-extension-to-telldontask.html, and I don't get how @paulodamaso wants to apply the decorator pattern here, see above. I really don't want to be rude to anybody, but to me the suggestion just doesn't make any sense. Maybe @paulodamaso and I are simply talking at cross purposes...

Regarding the test coverage: Agreed, working on it...

milux · 2020-02-20T17:23:36Z

@amihaiemil Unit tests implemented, overall coverage has increased. 😄
Ready for merge?

paulodamaso · 2020-02-20T17:36:11Z

@milux Don'ty worry about being rude, you aren't. Just for clarifying the idea about decorators. I was thinking in something like this:

public interface Containers extends Iterable<Container> {
	
}

public class Simple implements Container {
	public Iterator<Container> iterator() {
		//get all containers
	}
}

public class Filtered implements Containers {

	private final Map<String, Iterable<String>> filters;
	
	Filtered(final Map<String, Iterable<String>> newfilters){
		this.filters = newfilters;
	}

	Iterator<Container> iterator() {
		//filtering logic here, calling  Simple.iterator and filtering
	}
}

The idea became clearer? Does it make sense?

amihaiemil · 2020-02-20T17:48:41Z

@milux Vielen Dank!

However, Rultor has problems still. I am trying to fix the issue as soon as possible so we can release 0.0.11. See the latest opened Issue for the progress...

0crat · 2020-02-20T18:19:14Z

Job was finished in 29 hours, bonus for fast delivery is possible (see §36)

0crat · 2020-02-20T18:55:41Z

Order was finished: +20 point(s) just awarded to @paulodamaso /z

0crat · 2020-02-20T19:36:20Z

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @amihaiemil /z

Implemented more API complete container listing

1b7278f

0crat added scope role/REV labels Feb 19, 2020

paulodamaso suggested changes Feb 19, 2020

View reviewed changes

Added unit test for ListedContainers

899c08c

Got last LOC (all containers incl. size) covered

cfc428f

amihaiemil merged commit b1b1aed into amihaiemil:master Feb 20, 2020

0crat removed the scope label Feb 20, 2020

amihaiemil mentioned this pull request Feb 21, 2020

Release 0.0.11 #317

Closed

milux deleted the containers-listing branch February 24, 2020 16:33

Uh oh!

Implemented more API complete container listing #315

Implemented more API complete container listing #315

Uh oh!

Conversation

milux commented Feb 19, 2020

Uh oh!

coveralls commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 585

💛 - Coveralls

Uh oh!

milux commented Feb 19, 2020

Uh oh!

0crat commented Feb 19, 2020

Uh oh!

paulodamaso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milux Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amihaiemil commented Feb 19, 2020

Uh oh!

milux commented Feb 19, 2020

Uh oh!

milux commented Feb 19, 2020

Uh oh!

paulodamaso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amihaiemil commented Feb 20, 2020

Uh oh!

amihaiemil commented Feb 20, 2020

Uh oh!

milux commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milux commented Feb 20, 2020

Uh oh!

amihaiemil commented Feb 20, 2020

Uh oh!

milux commented Feb 20, 2020

Uh oh!

milux commented Feb 20, 2020

Uh oh!

paulodamaso commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 19, 2020 •

edited

Loading

milux Feb 20, 2020 •

edited

Loading

milux commented Feb 20, 2020 •

edited

Loading

paulodamaso commented Feb 20, 2020 •

edited

Loading