Skip to content
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

Continue filtering in ListedImages #154

Closed
amihaiemil opened this issue Aug 23, 2018 · 23 comments
Closed

Continue filtering in ListedImages #154

amihaiemil opened this issue Aug 23, 2018 · 23 comments
Labels
bug Something isn't working

Comments

@amihaiemil
Copy link
Owner

See puzzle.

@amihaiemil amihaiemil added the bug Something isn't working label Aug 23, 2018
@0crat
Copy link
Collaborator

0crat commented Aug 23, 2018

Job #154 is now in scope, role is DEV

@0crat
Copy link
Collaborator

0crat commented Aug 23, 2018

Bug was reported, see §29: +15 point(s) just awarded to @amihaiemil/z

@0crat
Copy link
Collaborator

0crat commented Sep 4, 2018

The job #154 assigned to @llorllale/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

@0crat
Copy link
Collaborator

0crat commented Sep 9, 2018

@llorllale/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

@llorllale
Copy link
Contributor

llorllale commented Sep 11, 2018

@amihaiemil

a map that would hold the actual filters

Why a map? What would be the keys and values of this map?

I understand now. Are you sure you want it implemented as a Map<String, String>? Since there are only a limited set of filters, wouldn't we want to have types defined for those?

Your answer fill probably be No but I want to make sure. :)

@llorllale
Copy link
Contributor

@0crat waiting for feedback

@0crat
Copy link
Collaborator

0crat commented Sep 11, 2018

@0crat waiting for feedback (here)

@llorllale The impediment for #154 was registered successfully by @llorllale/z

@llorllale
Copy link
Contributor

@amihaiemil aren't #153 and #154 asking for the same thing?

@amihaiemil
Copy link
Owner Author

amihaiemil commented Sep 13, 2018

@llorllale Yes, pretty much... I looked over this once, a few weeks ago, and I think I came to the conclusion that one of these tasks is obsolete. Could you refuse it until I figure it out? These days I am also rather busy and do not have as much time for OSS :(

@llorllale
Copy link
Contributor

@amihaiemil do you think there is a more elegant way forward than just overloading the Docker.images() method?

@amihaiemil
Copy link
Owner Author

@llorllale

do you think there is a more elegant way forward than just overloading the Docker.images() method?

I'm not sure what you mean by this :D
In this ticket we should add a Map of filters to the class ListedImages. The class should remain immutable, only add filters via the ctor -- that is, from Image.filter(...) we will simply new ListedImages(..., modifiedMap);. So actually this ticket and #153 are not the same thing.

I wanted to find a more elegant way since, like this, we will have that filtering map duplicated accross different implementations of Images... sadly, I couldn't come up with a suitable solution, so I say we do it like this and maybe decide on refactoring in the future.

Makes sense? :D

@llorllale
Copy link
Contributor

@amihaiemil I see.

So you want the solution to look like:

docker.images().filter(...) -> Images

.. instead of:

docker.images(filter) -> Images

@llorllale
Copy link
Contributor

llorllale commented Sep 21, 2018

@amihaiemil I think your solution may be viewed as inconsistent.

I would expect docker.images().filter(...) to also be reflected on the docker.images() instance that I already possess. Kinda weird, don't you think?

@amihaiemil
Copy link
Owner Author

amihaiemil commented Sep 22, 2018

@llorllale The initial Images you get from docker.images() basically contains all the images, with no filters applied. Then, if you want to apply some filters, you use Images.filter(...).

And, if you call Images.filter(...) multiple times, the final Images instance will indeed respect all of them. Passing the map of filters to the docker.images(...) method has a few problems IMO:

  1. you oblige the user to pass a Map (even an empty one) from the start, or you have to overload .images()

  2. SRP is a bit broken since now Docker knows something about filtering, which it is not supposed to. The docker client shouldn't care if the Images, Containers etc can or cannot be filtered.

  3. Filrering multiple times will cause procedural code for the client since they will have to manually fetch the existing filters of the Images they got, iterate over them and put them in a new Map, together with the new filters, which they will pass again to docker.images(...)

Makes sense? :D

@llorllale
Copy link
Contributor

@amihaiemil sure, it makes sense, it's just that it would've been nice if we could do

new Filter(
    docker.images()
);

And have an implementation smart enough to not fetch all images from the docker registry and then filter them in-memory.

But ok, let's not dwell to much and go forward with this.

@amihaiemil
Copy link
Owner Author

amihaiemil commented Sep 27, 2018

@llorllale Yes,

new Filter (
    docker.images()
);

(especially if this Filter is reusable on other entites such as Container) would be the most elegant, but currently the architecture does not support that (unless we fetch all the images in memory and filter them ourselves -- that's not ok). So yes, let's go forward with what we have, maybe we'll refactor later :D

llorllale added a commit to llorllale/docker-java-api that referenced this issue Sep 29, 2018
@llorllale
Copy link
Contributor

@0crat wait because I submitted PR #159 for this

llorllale added a commit to llorllale/docker-java-api that referenced this issue Sep 29, 2018
@0crat
Copy link
Collaborator

0crat commented Sep 29, 2018

@0crat wait because I submitted PR #159 for this (here)

@llorllale Job #154 is already on hold

@amihaiemil
Copy link
Owner Author

@0crat boost 6x

@0crat
Copy link
Collaborator

0crat commented Oct 3, 2018

@0crat boost 6x (here)

@amihaiemil Boost 6x was set for #154

@0crat
Copy link
Collaborator

0crat commented Oct 3, 2018

Boosting tasks is against our principles, see §15: -10 point(s) just awarded to @amihaiemil/z

@0crat
Copy link
Collaborator

0crat commented Oct 3, 2018

Order was finished: +90 point(s) just awarded to @llorllale/z

@0crat
Copy link
Collaborator

0crat commented Oct 3, 2018

The job #154 is now out of scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants