introduce config --images-to-build #8995

abdennour · 2021-12-04T10:41:34Z

What I did
Ability to filter images by services which uses built images
implement proposal of #8994 , exactly, option 1

Related issue

#8994

(not mandatory) A picture of a cute animal, if possible in relation with what you did

Ability to filter images by services which uses built images It solves docker#8994 ( option 2 ) Signed-off-by: abdennour <mail@abdennoor.com> Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

abdennour · 2021-12-04T10:55:01Z

@ndeloof

First-time contributors need a maintainer to approve running workflows.
please help to approve @ndeloof @glours

ndeloof · 2021-12-04T10:59:18Z

code looks good, but I'm not fan for adding an option that only applies to the --config pseudo-command (which should be a subcommand), as this would be confusing and not apply to other (pseudo-)commands.
Maybe time to introduce compose config images as a real subcommand which could define it's own options?

glours

It doesn't work as defined in the scenario 2 of #8995

> ./bin/docker-compose compose -f ~/sources/awesome-compose/nginx-golang/docker-compose.yml convert --only-built
services:
  backend:
    build:
      context: /Users/glours/sources/awesome-compose/nginx-golang/backend
      dockerfile: Dockerfile
    networks:
      default: null
  frontend:
    build:
      context: /Users/glours/sources/awesome-compose/nginx-golang/frontend
      dockerfile: Dockerfile
    depends_on:
      backend:
        condition: service_started
    networks:
      default: null
    ports:
    - mode: ingress
      target: 80
      published: 80
      protocol: tcp
networks:
  default:
    name: nginx-golang_default
> ./bin/docker-compose compose -f ~/sources/awesome-compose/nginx-golang/docker-compose.yml convert --volumes --only-built
~/sources/compose 8994-convert-built-images >

I was able to use the flag alone and I was also able to pass it to an unrelated flag such as --volumes

abdennour · 2021-12-04T11:38:55Z

Scenario 2 requires a lot of development @glours . For the time being, it can combine only with --images

Should i revert to Scenario 1 since it's very specific (--built-images) ?
Or Should i proceed with Scenario (2) by finishing the combination with other flags one by one ?
Or go with new scenario propose by @ndeloof above (docker compose config images ) ?

@glours @ndeloof any team decision, so i can proceed?

ndeloof · 2021-12-04T13:04:30Z

A possible issue with my proposed solution is that compose config accept a list of services as arguments, and introducing sub-commands would conflict with this usage or forbid use of some reserved names as services :'(

No strong opinion yet on the best approach, need to think twice about it

introduce config --images --only-built

abdennour · 2021-12-04T13:41:17Z

Yep! @ndeloof , Think about --only-built as adjective for services , as global option.
that time, filtering will happen at the level of project.Services or project.WithServices
Does it make sense!

ndeloof · 2021-12-04T13:57:24Z

Got it, make sense.

abdennour · 2021-12-04T14:15:11Z

Also, @ndeloof we can go agile : releasing config --built-images , then think about refactoring/re-organizing.

abdennour · 2021-12-04T18:00:01Z

@ndeloof @glours , since adding new flag depends on --images flag requires redesign, i just pushed now the option (1) which is a new flag without dependency to others

docker compose config --built-images

I would suggest to accept this feature as incremental release , and with motivation to go AGILE.
Later on, we can think about refactoring this subcommand , indeed, you have already --resolve-image-disgests, which should belong to subsubcommand images since long time

glours · 2021-12-04T18:33:23Z

@abdennour

I don't think we should accept this incremental step and I'll explain why
Compose is a wildy used project, if we introduce a new command or a new flag this new feature will be quickly used to script some bots or used in CI flows and then we'll be block (or it'll a least been painful) to remove this "temporary" increment.
This is typically the kind of behaviour which drive a project to feature overload.

I really prefer that we take few days to think to a proper solution instead of rushing.

If you're currently block by this missing option, maybe piping the json output format --format json with a jq filter can help you until we find the best option

ndeloof · 2021-12-04T18:41:57Z

I tend to agree. Agile != "let's add random stuff and change our mind any time", especially when it comes to UX. Once we introduce a new flag, it will be there forever.

abdennour · 2021-12-04T18:47:44Z

Totally agree @glours @ndeloof , this is INTERFACE design pattern & it should be stable by Design.
BTW, i am not blocked @glours , as i forked it and i released the feature in the fork: https://github.com/ElmCompany/compose/releases/tag/v2.2.4

Nevertheless, i will be waiting for the final design in #8994 , so we can go with it, and i would be happy to resume the contribution.
BTW, the software is lacking testing, let's work on it as well: May be capturing stdout for testing purposes will require extensive refactor from code perspective not functionality perspective.

It solves docker#8994 Signed-off-by: abdennour <mail@abdennoor.com>

abdennour · 2021-12-10T10:03:57Z

Reminder @ndeloof @glours

it solves docker#8994 Signed-off-by: abdennour <mail@abdennoor.com>

abdennour · 2021-12-17T17:56:20Z

@ndeloof @glours i made the change as per my comment here #8994 (comment)
It should be acceptable now.
Let me know if you need any change

abdennour · 2021-12-28T23:02:48Z

I really want to know if you need anything in order to approve/merge/release this PR.
@ndeloof @glours Kind reminder !!

abdennour · 2022-01-03T16:50:20Z

@glours You are requested for review.
@ndeloof who is the backup of @glours

abdennour · 2022-01-09T04:20:49Z

I really want to know if you need anything in order to approve/merge/release this PR.
@ndeloof @glours Kind reminder !!

glours · 2022-01-09T10:36:51Z

Hello @abdennour

Sorry for the delay of the response.
I still prefer the option of having a dedicated compose config images option mentioned by @ndeloof
With the holidays period we didn't have time to talk about this option internally ATM.
cc @ulyssessouza FYI

abdennour force-pushed the 8994-convert-built-images branch from 99ee7df to fcb774a Compare December 4, 2021 10:42

abdennour force-pushed the 8994-convert-built-images branch from fcb774a to 3bf943e Compare December 4, 2021 10:53

glours requested changes Dec 4, 2021

View reviewed changes

glours mentioned this pull request Dec 4, 2021

introduce config --images --only-built ElmCompany/compose#1

Merged

Merge pull request #1 from ElmCompany/8994-convert-built-images

27016ae

introduce config --images --only-built

abdennour force-pushed the 8994-convert-built-images branch from 3bf943e to accadb0 Compare December 4, 2021 17:55

abdennour requested a review from glours December 4, 2021 18:00

abdennour mentioned this pull request Dec 4, 2021

[Feature Request] "convert --images" list only built images #8994

Closed

introduce config --built-images

6b87c11

It solves docker#8994 Signed-off-by: abdennour <mail@abdennoor.com>

abdennour force-pushed the 8994-convert-built-images branch from accadb0 to 6b87c11 Compare December 4, 2021 19:04

abdennour changed the title ~~introduce config --images --only-built~~ introduce config --built-images Dec 5, 2021

refactor(config): introduce --images-to-build

7020503

it solves docker#8994 Signed-off-by: abdennour <mail@abdennoor.com>

abdennour force-pushed the 8994-convert-built-images branch from c4cb979 to 7020503 Compare December 17, 2021 17:55

abdennour changed the title ~~introduce config --built-images~~ introduce config --images-to-build Jan 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce config --images-to-build #8995

introduce config --images-to-build #8995

abdennour commented Dec 4, 2021 •

edited

Loading

abdennour commented Dec 4, 2021 •

edited by mat007

Loading

ndeloof commented Dec 4, 2021

glours left a comment •

edited

Loading

abdennour commented Dec 4, 2021 •

edited

Loading

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021

abdennour commented Dec 4, 2021

glours commented Dec 4, 2021 •

edited

Loading

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021 •

edited

Loading

abdennour commented Dec 10, 2021

abdennour commented Dec 17, 2021

abdennour commented Dec 28, 2021

abdennour commented Jan 3, 2022

abdennour commented Jan 9, 2022

glours commented Jan 9, 2022

introduce config --images-to-build #8995

Are you sure you want to change the base?

introduce config --images-to-build #8995

Conversation

abdennour commented Dec 4, 2021 • edited Loading

abdennour commented Dec 4, 2021 • edited by mat007 Loading

ndeloof commented Dec 4, 2021

glours left a comment • edited Loading

Choose a reason for hiding this comment

abdennour commented Dec 4, 2021 • edited Loading

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021

abdennour commented Dec 4, 2021

glours commented Dec 4, 2021 • edited Loading

ndeloof commented Dec 4, 2021

abdennour commented Dec 4, 2021 • edited Loading

abdennour commented Dec 10, 2021

abdennour commented Dec 17, 2021

abdennour commented Dec 28, 2021

abdennour commented Jan 3, 2022

abdennour commented Jan 9, 2022

glours commented Jan 9, 2022

abdennour commented Dec 4, 2021 •

edited

Loading

abdennour commented Dec 4, 2021 •

edited by mat007

Loading

glours left a comment •

edited

Loading

abdennour commented Dec 4, 2021 •

edited

Loading

glours commented Dec 4, 2021 •

edited

Loading

abdennour commented Dec 4, 2021 •

edited

Loading