Skip to content

Fix documentation on default build image name (change to correct separator).#11635

Merged
ndeloof merged 1 commit intodocker:mainfrom
sebastian-correa:fix-compose-build-docs
Mar 22, 2024
Merged

Fix documentation on default build image name (change to correct separator).#11635
ndeloof merged 1 commit intodocker:mainfrom
sebastian-correa:fix-compose-build-docs

Conversation

@sebastian-correa
Copy link
Copy Markdown
Contributor

@sebastian-correa sebastian-correa commented Mar 20, 2024 •
edited
Loading

What I did
This PR changes old documentation showing the old _ separator for image names.

Details:
150fd4b changed the default separator from _ to -, as seen in

compose/pkg/api/api.go

Lines 622 to 631 in 86cd523

var Separator = "-"
// GetImageNameOrDefault computes the default image name for a service, used to tag built images
func GetImageNameOrDefault(service types.ServiceConfig, projectName string) string {
imageName := service.Image
if imageName == "" {
imageName = projectName + Separator + service.Name
}
return imageName
}

but the compose build docs still mention _:

Screenshot 2024-03-19 at 23 11 42

Will this propagate downstream to docker/docs or do I need to submit specific PRs there for the updates?

A picture of a cute animal, if possible in relation to what you did

This should be mandatory. Sorry it's not related... This is my cat Panko being dracula.
panko_dracula

@sebastian-correa
Fix docs on default build image name
3a26f81 
150fd4b changed the default separator from _ to -, but the docker compose build docs still mention _.

Signed-off-by: Sebastian Correa <[email protected]>
ndeloof
ndeloof approved these changes Mar 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof 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 do you remember why you created #9687 ? What's the drawback using underscore separator in image names ?

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.68%. Comparing base (86cd523) to head (3a26f81).
Report is 7 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #11635      +/-   ##
==========================================
- Coverage   57.72%   57.68%   -0.05%     
==========================================
  Files         138      138              
  Lines       11736    11736              
==========================================
- Hits         6775     6770       -5     
- Misses       4284     4288       +4     
- Partials      677      678       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndeloof ndeloof merged commit 0191e69 into docker:main Mar 22, 2024
@sebastian-correa
Copy link
Copy Markdown
Contributor Author

sebastian-correa commented Mar 22, 2024

@ndeloof I think the change was made because compose v2 prefers the - separator so that service names are DNS valid. I know this has nothing to do with the image itself, but it was probably better (for consistency) and easier to have just 1 separator for everything.

@BrewTestBot BrewTestBot mentioned this pull request Mar 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndeloof ndeloof ndeloof approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sebastian-correa @ndeloof