Skip to content

WIP: feat(status) display failed_reason for a failing Kafka instance #476

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.

Sign up for GitHub

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

Merged
rkpattnaik780 merged 2 commits into from
Mar 22, 2021
Merged

WIP: feat(status) display failed_reason for a failing Kafka instance #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ff3c79
feat(status): display failed_reason for a failing Kafka instance
rkpattnaik780 Mar 22, 2021
0a14917
change logic of printservicestatus
rkpattnaik780 Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/rhoas/pkged.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion docs/commands/rhoas_serviceaccount_create.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ rhoas serviceaccount create [flags]
$ rhoas serviceaccount create

# create a service account and save the credentials in a Kubernetes secret file format
$ rhoas serviceaccount create --output kube
$ rhoas serviceaccount create --file-format kube

# create a service account and forcibly overwrite the credentials file if it exists already
$ rhoas serviceaccount create --overwrite
Expand Down
8 changes: 7 additions & 1 deletion locales/cmd/status/active.en.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,10 @@ one = 'Format in which to display the status of your services. Choose from: "jso
one = 'Requesting status of the following services:'

[status.log.info.noStatusesAreUsed]
one = 'No services are currently being used. To set a service in context, run "rhoas <service> use [args]".'
one = 'No services are currently being used. To set a service in context, run "rhoas <service> use [args]".'

[status.log.debug.noKafkaSelected]
one = 'No Kafka instance is currently used, skipping status check'

[status.log.info.selectAnotherKafka]
one = 'Run rhoas kafka use --id=<kafka-instance-id> to use another Kafka instance.'
34 changes: 26 additions & 8 deletions pkg/status/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import (
"fmt"
"io"
"reflect"
"strings"
"text/tabwriter"

"github.com/bf2fc6cc711aee1a0c2a/cli/pkg/connection"
"github.com/bf2fc6cc711aee1a0c2a/cli/pkg/kafka"

"github.com/bf2fc6cc711aee1a0c2a/cli/pkg/color"

"github.com/bf2fc6cc711aee1a0c2a/cli/internal/config"
"github.com/bf2fc6cc711aee1a0c2a/cli/internal/localizer"
kas "github.com/bf2fc6cc711aee1a0c2a/cli/pkg/api/kas"
kasclient "github.com/bf2fc6cc711aee1a0c2a/cli/pkg/api/kas/client"
"github.com/bf2fc6cc711aee1a0c2a/cli/pkg/logging"
Expand All @@ -30,6 +30,7 @@ type KafkaStatus struct {
Name string `json:"name,omitempty"`
Status string `json:"status,omitempty"`
BootstrapServerHost string `json:"bootstrap_server_host,omitempty" title:"Bootstrap URL"`
FailedReason string `json:"failed_reason,omitempty" title:"Failed Reason"`
Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Mar 22, 2021

Choose a reason for hiding this comment

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

omitempty is not getting rid of the FailedReason key when the value is "". Same goes with other fields(tested by hardcoding).

Copy link
Contributor

@craicoverflow craicoverflow Mar 22, 2021

Choose a reason for hiding this comment

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

Okay, we will need to update the status printing logic in printServiceStatus to skip the field if omitempty is set and the value is empty.

}

type Options struct {
Expand Down Expand Up @@ -64,14 +65,14 @@ func Get(ctx context.Context, opts *Options) (status *Status, ok bool, err error
if kas.IsErr(err, kas.ErrorNotFound) {
err = kafka.ErrorNotFound(kafkaCfg.ClusterID)
logger.Info(err)
logger.Info("Run", color.CodeSnippet("rhoas kafka use --id=<kafka-instance-id>"), "to use another Kafka instance.")
logger.Info(localizer.MustLocalizeFromID("status.log.info.selectAnotherKafka"))
}
} else {
status.Kafka = kafkaStatus
ok = true
}
} else {
logger.Debug("No Kafka instance is currently used, skipping status check")
logger.Debug(localizer.MustLocalizeFromID("status.log.debug.noKafkaSelected"))
Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Mar 22, 2021

Choose a reason for hiding this comment

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

Shouild this be logger.Info instead? The user should be able to see this message irrespective of passing the --debug flag, what do you think @craicoverflow

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Mar 22, 2021

Choose a reason for hiding this comment

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

What convention should we follow to localize strings splitted like this?

logger.Info("Run", color.CodeSnippet("rhoas kafka use --id=<kafka-instance-id>"), "to use another Kafka instance.")

Copy link
Contributor

@craicoverflow craicoverflow Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think they need to see that information all the time.

If I use rhoas for only one service "A", and there are 25 other services unused called "B" - "Z", you would get something like this:

$ rhoas status

A
-------------------
Blah:
Blah:
Blah:
Blah:

No B instance is currently used, skipping status check
No C instance is currently used, skipping status check
No D instance is currently used, skipping status check
No E instance is currently used, skipping status check
No F instance is currently used, skipping status check
No G instance is currently used, skipping status check
No H instance is currently used, skipping status check
No I instance is currently used, skipping status check
No J instance is currently used, skipping status check
No K instance is currently used, skipping status check
No L instance is currently used, skipping status check
No M instance is currently used, skipping status check
...

"No B instance is currently used, skipping status check" does not tell the user much more than they can already figure out by there being no status information about the service in the output, but it is useful if they know that this service is currently used, they can check if it is being skipped, which can be useful to debug the problem.

Copy link
Contributor

@craicoverflow craicoverflow Mar 22, 2021

Choose a reason for hiding this comment

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

What convention should we follow to localize strings splitted like this?

logger.Info("Run", color.CodeSnippet("rhoas kafka use --id=<kafka-instance-id>"), "to use another Kafka instance.")

This was only split because color.CodeSnippet was used so it was easier to mix into the string. The alternative was to use log.Printf.

Since we are using localization now, there will be no need for splitting the strings like this as the full sentence is formed in the locales file.

}
}

Expand Down Expand Up @@ -120,10 +121,12 @@ func printServiceStatus(w io.Writer, name string, v reflect.Value) {
// get the title to use for the field
title := getTitle(&fieldType)

// print the row and take note of its character length
len, _ := fmt.Fprintf(tw, "%v:\t\t%v\n", title, fieldValue)
if len > maxRowLen {
maxRowLen = len
if !getOmitEmpty(&fieldType) || !fieldValue.IsZero() {
// print the row and take note of its character length
len, _ := fmt.Fprintf(tw, "%v:\t\t%v\n", title, fieldValue)
if len > maxRowLen {
maxRowLen = len
}
}
}
// print the service header
Expand All @@ -146,6 +149,17 @@ func getTitle(f *reflect.StructField) string {
return tag
}

// check if omitempty is set
func getOmitEmpty(f *reflect.StructField) bool {
var omitempty bool
tag := f.Tag.Get("json")
if tag != "" {
omitempty = strings.Contains(tag, "omitempty")
}

return omitempty
}

// create a divider for the top of the table of n length
func createDivider(n int) string {
b := "-"
Expand All @@ -172,6 +186,10 @@ func getKafkaStatus(ctx context.Context, api kasclient.DefaultApi, id string) (s
BootstrapServerHost: kafkaResponse.GetBootstrapServerHost(),
}

if kafkaResponse.GetStatus() == "failed" {
status.FailedReason = kafkaResponse.GetFailedReason()
}

return status, err
}

Expand Down