compose icon indicating copy to clipboard operation
compose copied to clipboard

introduce config --images-to-build

Open abdennour opened this issue 4 years ago • 17 comments

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

abdennour avatar Dec 04 '21 10:12 abdennour

@ndeloof

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

image

abdennour avatar Dec 04 '21 10:12 abdennour

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?

ndeloof avatar Dec 04 '21 10:12 ndeloof

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

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

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

abdennour avatar Dec 04 '21 11:12 abdennour

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

ndeloof avatar Dec 04 '21 13:12 ndeloof

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!

abdennour avatar Dec 04 '21 13:12 abdennour

Got it, make sense.

ndeloof avatar Dec 04 '21 13:12 ndeloof

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

abdennour avatar Dec 04 '21 14:12 abdennour

@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

abdennour avatar Dec 04 '21 18:12 abdennour

@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

glours avatar Dec 04 '21 18:12 glours

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.

ndeloof avatar Dec 04 '21 18:12 ndeloof

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.

abdennour avatar Dec 04 '21 18:12 abdennour

Reminder @ndeloof @glours

abdennour avatar Dec 10 '21 10:12 abdennour

@ndeloof @glours i made the change as per my comment here https://github.com/docker/compose/issues/8994#issuecomment-996905644 It should be acceptable now. Let me know if you need any change

abdennour avatar Dec 17 '21 17:12 abdennour

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

abdennour avatar Dec 28 '21 23:12 abdennour

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

abdennour avatar Jan 03 '22 16:01 abdennour

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

abdennour avatar Jan 09 '22 04:01 abdennour

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

glours avatar Jan 09 '22 10:01 glours