fixed error capitalization

[devang-gaur]

fixes #637

[knrt10]

+1. Thanks @dev-gaur for your contribution

[invidian]

Thanks for your contribution @dev-gaur. Before we can merge, please fix new sentences in the error messages, they should start with capital letter.

While we touch error messages, it would also be nice to re-phrase them properly, as we discuss, even though the decision is not finalized yet. See #953 and #883 for more details. TL;DR we would also remove failed, error etc keywords from error messages, leaving only for example checking Terraform version, as the fact that this is the error is implied. I posted 2 suggestions how it should look like.
However, this is of course optional for your work, as the changes presented in the PR are already a nice improvement :)

[invidian]

Suggested change

	return fmt.Errorf("parsing Terraform version failed")
	return fmt.Errorf("parsing Terraform version")

[devang-gaur]

@invidian now the string parsing Terraform version will look like a log message instead of an error message.
how about error occured while parsing Terraform version or unable to parse Terraform version?

[knrt10]

this might help. Please check https://github.com/kinvolk/lokomotive/pull/953/files#diff-a4a1697930a32405cd5efdc7aa09b71fR38

[invidian]

Hmm, now that I think about it, this error message doesn't seem helpful at all. We could include expected format and parsed value in it.

Also, I look at the documentation of fmt.Sscanf right now and try to figure out in which case it returns error here.

Looking at https://golang.org/src/fmt/scan.go?s=4522:4597#L1181, I think it will always return err if n is different than number of operands passed. Would be good to have some unit tests for it :/

[invidian]

LGTM

[invidian]

Actually, before we close #637, we should also fix cli/cmd/cluster-destroy.go and cli/cmd/cluster-apply.go context logger messages, which are not capitalized at the moment.

[devang-gaur]

@invidian do you mean contextLogger.Fatalf messages only ? or do you want to include contextLogger.Println messages as well?

[invidian]

@dev-gaur it seems all Println occurences are already capitalized. I only see 2 lines with not capitalized Fatalf:

• error applying cluster
• error destroying cluster

[devang-gaur]

@dev-gaur it seems all Println occurences are already capitalized. I only see 2 lines with not capitalized Fatalf:

• error applying cluster
• error destroying cluster

Aah I see. I thought you wanted to remove capitalization..

I have a confusion:
FatalF messages are technically exiting errors... So why do we want to capitalize them?

[invidian]

FatalF messages are technically exiting errors... So why do we want to capitalize them?

Because those are the messages, which gets printed to the user, so they should be capitalized.

[devang-gaur]

FatalF messages are technically exiting errors... So why do we want to capitalize them?

Because those are the messages, which gets printed to the user, so they should be capitalized.

Thanks for clarifying. Changes made.

[invidian]

Looks awesome now @dev-gaur. Thanks a lot for your effort!

[knrt10]

+1. Thanks again for the effort.

[devang-gaur]

even I learnt a bit about error formatting :)
thanks for attending to my PR so promptly