builder: move builder creation logic to builder package #2150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crazy-max
force-pushed
the
builder-create
branch
from
2f87d1c to
61cfb22
Compare
crazy-max
commented
Dec 4, 2023
| if opts.Name == "default" { | ||
| return nil, errors.Errorf("default is a reserved name and cannot be used to identify builder instance") | ||
| } else if opts.Append && opts.Name == "" { | ||
| return nil, errors.Errorf("append requires a builder name") |
There was a problem hiding this comment.
Now returns an error instead of a warning log:
Line 67 in b2f705a
I think being more strict about it is fine but let me know if we should typed this kind error and handle it it differently in command package when invoked through the CLI to still log a warning or just pass a logger to Create.
crazy-max
commented
Dec 4, 2023
| } | ||
| for _, c := range contexts { | ||
| if c.Name == name { | ||
| return nil, errors.Errorf("instance name %q already exists as context builder", name) |
There was a problem hiding this comment.
Same as #2150 (comment)
crazy-max
force-pushed
the
builder-create
branch
from
61cfb22 to
c9daf3d
Compare
crazy-max
commented
Dec 4, 2023
| switch { | ||
| case driverName == "kubernetes": | ||
| if opts.Endpoint != "" { | ||
| return nil, errors.Errorf("kubernetes driver does not support endpoint args %q", opts.Endpoint) |
There was a problem hiding this comment.
Same as #2150 (comment)
crazy-max
commented
Dec 4, 2023
| err2 = txn.Save(ngOriginal) | ||
| } | ||
| if err2 != nil { | ||
| return nil, errors.Errorf("could not rollback to previous state: %s", err2) |
There was a problem hiding this comment.
Same as #2150 (comment)
This moves the builder creation logic to the builder package as we plan to allow builder creation in the build ui of Docker Desktop so we avoid drifting with the same logic in command package. Signed-off-by: CrazyMax <[email protected]>
crazy-max
force-pushed
the
builder-create
branch
from
c9daf3d to
ceb5bc8
Compare
tonistiigi
approved these changes
Dec 11, 2023
| var ep string | ||
| var setEp bool | ||
| switch { | ||
| case driverName == "kubernetes": |
There was a problem hiding this comment.
Ideally, these driver specifics can be removed from this package eventually, but not new code as this is how it was in commands as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the builder creation logic to the builder package as we plan to allow builder creation in the build UI of Docker Desktop so we avoid drifting with the same logic in
commandpackage.