Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

When none set, generate a random installation name #609

Merged
merged 1 commit into from Oct 1, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Sep 9, 2019

- What I did

Generate a random installation name when none is set by user for uniformity with docker run user experience.

- How I did it
Vendored dockerd namegenerator.
Clean implementation would require engine to prevent name conflicts, would make sense once we have an engine-side "deploy" implementation

- How to verify it
docker app install without a name, then docker app ls

- Description for the changelog
A random installation name is generated when none is passed to docker app install.

- A picture of a cute animal (not mandatory but encouraged)
image

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@ndeloof ndeloof marked this pull request as ready for review September 9, 2019 14:56
@codecov
Copy link

codecov bot commented Sep 9, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5b857bc). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #609   +/-   ##
=========================================
  Coverage          ?   71.54%           
=========================================
  Files             ?       51           
  Lines             ?     2611           
  Branches          ?        0           
=========================================
  Hits              ?     1868           
  Misses            ?      500           
  Partials          ?      243
Impacted Files Coverage Δ
internal/commands/install.go 66.19% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b857bc...0df017d. Read the comment docs.

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

@silvin-lubecki
Copy link
Contributor

silvin-lubecki commented Sep 9, 2019

Is there a way to test this feature, or at least indirectly? 🤔

@jcsirot
Copy link
Contributor

jcsirot commented Sep 19, 2019

The description of the name flag line needs to be updated since the default behavior changed

cmd.Flags().StringVar(&opts.stackName, "name", "", "Installation name (defaults to application name)")

@ndeloof
Copy link
Contributor Author

ndeloof commented Sep 26, 2019

@thaJeztah do you think this make sense or shall we better wait for an engine API to install apps and manage unique random name generation ?

For uniformity with `docker run` user experience.
Note a full implementation would required to get the engine manage
random name unicity, like it does for containers.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours merged commit fb71be0 into docker:master Oct 1, 2019
@ndeloof ndeloof deleted the defaultname branch October 1, 2019 08:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants