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

Add new naming strategy "auto" #944

Closed
wants to merge 1 commit into from
Closed

Add new naming strategy "auto" #944

wants to merge 1 commit into from

Conversation

marcust
Copy link
Contributor

@marcust marcust commented Feb 14, 2018

Enable a new default naming strategy "auto" to create container names based on
a prefix and the image simple name. Adding a global configration for the naming
strategy in order to change it on a global level and thus closing #931.

I don't know if that is a little bit over the top or very narrow minded from our use case point of view, feedback is more than welcome.

Enable a new default naming strategy "auto" to create container names based on
a prefix and the image simple name. Adding a global configration for the naming
strategy in order to change it on a global level and thus closing #931.
@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #944 into master will increase coverage by <.01%.
The diff coverage is 35.89%.

@@             Coverage Diff              @@
##             master     #944      +/-   ##
============================================
+ Coverage     51.57%   51.58%   +<.01%     
- Complexity     1232     1237       +5     
============================================
  Files           144      146       +2     
  Lines          7251     7270      +19     
  Branches        981      983       +2     
============================================
+ Hits           3740     3750      +10     
- Misses         3195     3203       +8     
- Partials        316      317       +1
Impacted Files Coverage Δ Complexity Δ
.../io/fabric8/maven/docker/service/WatchService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rc/main/java/io/fabric8/maven/docker/StopMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/io/fabric8/maven/docker/service/RunService.java 52.31% <0%> (+1.18%) 25 <0> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/StartMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/WatchMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ic8/maven/docker/config/RunImageConfiguration.java 90.57% <100%> (-0.21%) 37 <1> (-1)
...io/fabric8/maven/docker/config/NamingStrategy.java 100% <100%> (ø) 1 <1> (?)
...va/io/fabric8/maven/docker/util/ContainerName.java 69.23% <69.23%> (ø) 5 <5> (?)

@rhuss
Copy link
Collaborator

rhuss commented Feb 20, 2018

Thanks a lot ! I'm currently on the road (and was on PTO last week), but I will have a look as soon as I'm back (early next week)

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the use case and I think we should support a more flexible way for naming containers.

However I'm a bit afraid of configuration bloat (as always ;-). E.g. blowing up the parameter list to 6 parameters is going in the wrong direction.

As the container prefix is only used for the naming strategy, maybe we could combine them into a single configuration object (and then also leave the calculation of the name to this object). this could be move around if necessary.

E.g. I could imagine to have a

<containerNaming>
   <strategy>auto</strategy>
   <prefix>...</prefix>
</containerNaming>

(the name of the tag is discussable but we can't reuse namingStrategy because of backwards compatibility, but we could deprecate it and eventually remove it)

wdyt ?

Actually, we could go even a bit further and allow a pattern for the name which e.g. placeholder for timestamps (%t), the image name (%n), the alias (%a) or any other, image specific parameter. Using timestamps would have the benefit that the same run can be run multiple times against the same docker daemon. Even a strategy like "incremental" could be used to add a numeric suffix to the end of the name if the container already exists.

Thinking again, maybe we should drop the <namingStrategy> completely and replace it with a single <containerNamePattern> which would be

  • %a for the "alias" mode
  • %n for the simple name
  • anyprefix-%n works, too
  • %n-%t name + timestamp
  • %n%i for name, name-1, name-2, ... depending on whether the container already exists.
  • If nothing has been configured the the container wont be named (maybe for overriding a default we would need a special keyword)

That way we could with a single parameter flexibly match a lot of use case.

You see, we can go even more over the top ;-) (well, actually its still only a small change in the configuration, but with a big effect)

wdyt ?

btw, I'm going to make a release now, so we might get this in in one of the next releases.

@marcust
Copy link
Contributor Author

marcust commented Apr 10, 2018

@rhuss Well, in general I think a more flexible naming system would be a good thing, though I basically only have the use-case I integrated.

The reason I just added it was that I didn't want to break backward compatibility too much, because we have like 50 projects using it like it is now and it would be easier to just change the default strategy here.

Anyways, I might find some time tomorrow too have a look at implementing your proposal, the more I think about it the more I like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants