Add --force-color flag

[sarthaksarthak9]

Add --force-color flag to force color even when the output is not a TTY.

Summary

Output

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #1839

[sarthaksarthak9]

@jjbustamante (I don't think so this error coming because of the changes I made)

sarthak@hp-pavilion:~/pack$ make verify
=====> Installing goimports...
cd tools && go install golang.org/x/tools/cmd/goimports
=====> Verifying format...
=====> Installing golangci-lint...
cd tools && go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.1
=====> Linting code...
internal/sshdialer/ssh_dialer.go:141:3: naked return in func networkAndAddressFromRemoteDockerHost with 49 lines of code (nakedret)
return
^

[sarthaksarthak9]

here we can see in flags force-color is showing
CLI for building apps using Cloud Native Buildpacks

Usage:
pack [command]

Available Commands:
build Generate app image from source code
builder Interact with builders
buildpack Interact with buildpacks
extension Interact with extensions
config Interact with your local pack config file
inspect Show information about a built app image
stack (deprecated) Interact with stacks
rebase Rebase app image with latest run image
sbom Interact with SBoM
completion Outputs completion script location
report Display useful information for reporting an issue
version Show current 'pack' version
help Help about any command

Flags:
--force-color Force color output
-h, --help Help for 'pack'
--no-color Disable color output
-q, --quiet Show less output
--timestamps Enable timestamps in output
-v, --verbose Show more output
--version Show current 'pack' version

[jjbustamante]

Hi @sarthaksarthak9

Could you run make format ?

[sarthaksarthak9]

Hi @sarthaksarthak9

Could you run make format ?

okk

[jjbustamante]

I think the code is still not working as expected.

I know there are not unit test in this file, but maybe you could also add a new unit test to confirm the new flag is actually doing what you are expecting.

[jjbustamante]

Hi @sarthaksarthak9

If you use the --force-color flag, you will notice it will never reach this line. the variable declared on L43 is always set to false and you are never reading the flag value.

Hint: check L53

[sarthaksarthak9]

Hi @sarthaksarthak9

If you use the --force-color flag, you will notice it will never reach this line. the variable declared on L43 is always set to false and you are never reading the flag value.

Hint: check L53

@jjbustamante Thank you, I will check this out!!

[sarthaksarthak9]

I am planning to do this in this way !!! Is this approach right ?

[jjbustamante]

Yes! just use the fs.GetBool function to validate if the flag was enabled by the end user. I think you don't need the forceColor flag anymore, right?

[sarthaksarthak9]

Yah, that's true there is no longer need to define --force-color explicitly as logic checks directly if the user has provided the --force-color flag using fs.GetBool

[jjbustamante]

Wait! its fine to remove:

var forceColor bool

but you still need:

rootCmd.PersistentFlags().Bool("force-color", false, "Force color output")

remember, adding the flag, is the only way you have to allow users to set or not the flag, when the user set --force-color then fs.GetBool("force-color") will return true.

Also, you used to have:

if force-color {
   color.Enabled() 
} else {
     if no-color { 
         color.Disable(true)
     } else {
         // more logic
     }
}

but you changed it to:

if force-color {
   color.Enabled() 
} 
if no-color { 
   color.Disable(true)
} else {
   // more logic
}

I think you will have troubles if run pack commands with both --force-color and --no-color, I know it is a crazy scenario, but I think the original flow is better.

[sarthaksarthak9]

ohh, thanks a lot !! I will take care of this !!

[jjbustamante]

Hi @sarthaksarthak9.

I apologized for all the back and forth, I think both of us are learning something about it!.

I was checking how the color.Disable(bool) and color.Enable() methods works, and I think we can do just one more adjust here.

By default, color is enabled, and we are just disabling it when --no-color or is not a terminal, I think what we can do is to do what we are doing today if --force-color is not the expected behavior. Something like:

if forceColor, err := fs.GetBool("force-color"); err == nil && !forceColor {
	if flag, err := fs.GetBool("no-color"); err == nil && flag {
		color.Disable(flag)
	}

	_, canDisplayColor := term.IsTerminal(logging.GetWriterForLevel(logger, logging.InfoLevel))
	if !canDisplayColor {
		color.Disable(true)
	}
}

[sarthaksarthak9]

okk, I will try my best

[sarthaksarthak9]

@jjbustamante R the changes still not working yet?

[jjbustamante]

@jjbustamante R the changes still not working yet?

Hi @sarthaksarthak9 I just tested it and it works!.

Here is how I did it:

• I compiled your branch
• I tried to build the sample ruby app using the following command

>  true | out/pack build --force-color my-app -p <path-to-samples>/apps/ruby-bundler -b <path-to-samples>/buildpacks/ruby-bundler 2>&1 | cat

Even thought I was not able to build the application, we can see in the following image, colors were used

[jjbustamante]

Thanks @sarthaksarthak9 for your contribution!

[sarthaksarthak9]

@jjbustamante R the changes still not working yet?

Hi @sarthaksarthak9 I just tested it and it works!.

Here is how I did it:

• I compiled your branch
• I tried to build the sample ruby app using the following command

>  true | out/pack build --force-color my-app -p <path-to-samples>/apps/ruby-bundler -b <path-to-samples>/buildpacks/ruby-bundler 2>&1 | cat

Even thought I was not able to build the application, we can see in the following image, colors were used

Thank you for guiding me