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

build: change --console=[auto,false,true] to --progress=[auto,plain,tty] #1276

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

tiborvass
Copy link
Collaborator

This changes the experimental --console flag to --progress following
feedback indicating avoidable confusion.

In addition to naming changes, the help output now has an additional
clarification, specifically: container output during builds are only
shown when progress output is set to plain. Not mentioning this was also
a big cause of confusion.

Signed-off-by: Tibor Vass tibor@docker.com

opts/opts.go Outdated
@@ -308,6 +307,16 @@ func ValidateSysctl(val string) (string, error) {
return "", fmt.Errorf("sysctl '%s' is not whitelisted", val)
}

func ValidateProgressOutput(val string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need to return a string here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's to respect the other validation Validate* signatures, that are able to mutate the value being validated. As we don't really use it into a type that require this signature (and as I'm not found of a validation function mutating the value it validates), I think we shouldn't return a string and just an error

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Design LGTM
Small comment on the code, and there is a linter issue too 👼

opts/opts.go Outdated
@@ -308,6 +307,16 @@ func ValidateSysctl(val string) (string, error) {
return "", fmt.Errorf("sysctl '%s' is not whitelisted", val)
}

func ValidateProgressOutput(val string) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's to respect the other validation Validate* signatures, that are able to mutate the value being validated. As we don't really use it into a type that require this signature (and as I'm not found of a validation function mutating the value it validates), I think we shouldn't return a string and just an error

@tiborvass tiborvass force-pushed the buildkit-progress-flag branch from 60be7af to 31fccd0 Compare August 7, 2018 18:17
@codecov-io
Copy link

Codecov Report

Merging #1276 into master will increase coverage by 0.02%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
+ Coverage   54.29%   54.32%   +0.02%     
==========================================
  Files         268      268              
  Lines       17855    17847       -8     
==========================================
  Hits         9695     9695              
+ Misses       7550     7542       -8     
  Partials      610      610

This changes the experimental --console flag to --progress following
feedback indicating avoidable confusion.

In addition to naming changes, the help output now has an additional
clarification, specifically: container output during builds are only
shown when progress output is set to plain. Not mentioning this was also
a big cause of confusion.

Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the buildkit-progress-flag branch from 31fccd0 to faeb8bb Compare August 7, 2018 18:18
@tiborvass
Copy link
Collaborator Author

@vdemeester @AntaresS fixed!

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Copy link
Contributor

@AntaresS AntaresS left a comment

Choose a reason for hiding this comment

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

LGTM

@tiborvass tiborvass merged commit 9641739 into docker:master Aug 16, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.09.0 milestone Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants