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

Remove leftover log level handlings #824

Merged
merged 6 commits into from
Nov 9, 2022
Merged

Conversation

godrei
Copy link
Contributor

@godrei godrei commented Oct 17, 2022

Checklist

Version

Requires a PATCH version update

Context

This PR removes leftover log level handling related code.
The log level flag was removed in this PR: https://github.com/bitrise-io/bitrise/pull/818/files#diff-4d5ebe4cc8e9de729d43f8f013c4b4a34bf473aed074c823c9178fb8f8938168

Changes

  • Update debug flag description
  • Remove LOGLEVEL env var
  • Don't specify --loglevel for external tools

Investigation details

Decisions

@godrei godrei marked this pull request as ready for review October 21, 2022 08:54
@@ -160,13 +160,13 @@ func StepmanStepInfo(collection, stepID, stepVersion string) (stepmanModels.Step

// StepmanRawStepList ...
func StepmanRawStepList(collection string) (string, error) {
args := []string{"--loglevel", logLevel(), "step-list", "--collection", collection, "--format", "raw"}
args := []string{"step-list", "--collection", collection, "--format", "raw"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The user can set the debug mode in two ways:

  • env var
  • command line parameter

The stepman process inherits the envs from the parent process so if the user used the debug env key then stepman can also print the debug logs.

But if the user used the command line parameters then the cli will print debug logs but stepman will not because we are not passing the loglevel anymore.

I do not know yet how useful the stepman debug logs are so I am also happy to remove this. But if we want consistency then I would add the --loglevel debug parameter to stepman in the case if we are running in a debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated my PR to restore the original behaviour around debug mode.
Now envan and stepman inherits Bitrise CLIs debug mode, this involved 2 main changes:

  • improved how we parse debug mode flag (loggerParameters) + tests
  • setting LOGLEVEL=debug env var

Specifying --loglevel=debug flag for the envman and stepman commands is not needed because the LOGLEVEL env var controls the log level flag for both stepman and envman.

On the other side, specifying --loglevel=debug in itself is not enough, because this way we restore the behavior for wrapped commands of envman and stepman, but not for the envman and stepman plugin commands:

  • Plugin command: bitrise -debug stepman share audit
  • Wrapped command: bitrise -debug share audit

@@ -195,3 +112,108 @@ func Run() {
failf(err.Error())
}
}

func loggerParameters(arguments []string) (isRunCommand bool, outputFormat log.LoggerType, isDebug bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very nice upgrade you did here.

I looked at the code and saw that the if statements do the same thing (or similar) regarding how they set the values. I think we can group them and an implementation like this:

func loggerParameters(arguments []string) (isRunCommand bool, outputFormat log.LoggerType, isDebug bool) {
	for i, argument := range arguments {
		if argument == "run" {
			isRunCommand = true
		}

		// syntax
		// -flag
		// --flag   // double dashes are also permitted
		// -flag=x
		// -flag x  // non-boolean flags only
		// One or two dashes may be used; they are equivalent.
		// https://pkg.go.dev/flag#hdr-Command_line_flag_syntax
		if strings.HasPrefix(argument, "--"+OutputFormatKey) || strings.HasPrefix(argument, "-"+OutputFormatKey) {
			var value string
			components := strings.Split(argument, "=")

			// If the flag value was specified with an `=` mark then the second element in the array is the actual value.
			// Otherwise, the value was specified as a separate item after the flag, and we need to take the next
			// argument value.
			if len(components) == 2 {
				value = components[1]
			} else if i+1 < len(arguments) {
				value = arguments[i+1]
			}

			switch value {
			case string(log.JSONLogger):
				outputFormat = log.JSONLogger
			case string(log.ConsoleLogger):
				outputFormat = log.ConsoleLogger
			default:
				// At this point we don't care about invalid values,
				// the execution will fail when parsing the command's arguments.
			}
		}

		if strings.HasPrefix(argument, "--"+DebugModeKey) || strings.HasPrefix(argument, "-"+DebugModeKey) {
			components := strings.Split(argument, "=")
			if len(components) == 2 {
				value, err := strconv.ParseBool(components[1])
				if err == nil {
					isDebug = value
				}
			} else {
				components := strings.Split(argument, " ")

				// "-flag x" Command line flag syntax is not supported for boolean flags
				// https://pkg.go.dev/flag#hdr-Command_line_flag_syntax
				if len(components) == 1 {
					isDebug = true
				}
			}
		}
	}

	return
}

if err := plugins.InitPaths(); err != nil {
failf("Failed to initialize plugin path, error: %s", err)
}

cli.VersionPrinter = printVersion
cli.VersionPrinter = func(c *cli.Context) { log.Print(c.App.Version) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@godrei godrei merged commit 55ac240 into master Nov 9, 2022
@godrei godrei deleted the remove-log-level-flag-handling branch November 9, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants