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

Allow overriding json values before sending #644

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,12 @@ CLUSTER_ID= # the unique cluster name, or internal, external id for a cluster
TEMPLATE= # file or url in which the template exists in
osdctl servicelog post ${CLUSTER_ID} --template=${TEMPLATE} --dry-run

# Post an internal-only service log message
osdctl servicelog post ${CLUSTER_ID} -i -p "MESSAGE=This is an internal message" --dry-run

# Post a short external message
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but here under "external message", the command has a lot of "internal message" content for the summary and description. Not a real issue, but might confuse someone

Copy link
Member

@clcollins clcollins Jan 22, 2025

Choose a reason for hiding this comment

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

Actually, I'm not quite sure why you can override the -i (--internal) here, with -r internal_only=False. That seems unintuitive. That might be a good use case to say something like "-i is incompatible with -r internal_only=false". Definitely an edge case, though.

Can you elaborate?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to avoid adding another command line argument (like -e) that includes a blank template, or a template with some sane defaults. As we discussed lets come back and add it in the future if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it would be a bit confusing if we have to use -i to combine with the -r. I will make me think the override works for internal messages only.
Could it be possible we just can make the template optional if the -r specified?

Copy link
Author

@smarthall smarthall Jan 23, 2025

Choose a reason for hiding this comment

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

Okay I'll make a change to that if neither -t or -i is specified, and at least one -r option is supplied the following template is used by default:

{
	"severity": "Info",
	"service_name": "SREManualAction",
	"internal_only": true
}

This way the user has to explicitly override the summary and description. Then if they want to send it to the customer they also have to explicitly override the internal_only variable.

osdctl servicelog post ${CLUSTER_ID} -r "summary=External Message" -r "description=This is an external message" -r internal_only=False --dry-run

QUERIES_HERE= # queries that can be run on ocm's `clusters` resource
TEMPLATE= # file or url in which the template exists in
osdctl servicelog post --template=${TEMPLATE} --query=${QUERIES_HERE} --dry-run
Expand Down
3 changes: 2 additions & 1 deletion cmd/servicelog/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@ package servicelog
import (
"encoding/json"
"fmt"
"time"

sdk "github.com/openshift-online/ocm-sdk-go"
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
v1 "github.com/openshift-online/ocm-sdk-go/servicelogs/v1"
"github.com/openshift/osdctl/internal/servicelog"
"github.com/openshift/osdctl/pkg/utils"
"time"
)

var (
Expand Down
109 changes: 105 additions & 4 deletions cmd/servicelog/post.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ package servicelog

import (
"encoding/json"
"errors"
"fmt"
"net/url"
"os"
"os/signal"
"path/filepath"
"reflect"
"regexp"
"strconv"
"strings"
"time"

Expand All @@ -32,6 +35,7 @@ type PostCmdOptions struct {
ClustersFile servicelog.ClustersFile
Template string
TemplateParams []string
Overrides []string
filterFiles []string // Path to filter file
filtersFromFile string // Contents of filterFiles
filterParams []string
Expand Down Expand Up @@ -63,6 +67,12 @@ func newPostCmd() *cobra.Command {
# Post a service log to a single cluster via a remote URL, providing a parameter
osdctl servicelog post ${CLUSTER_ID} -t https://raw.githubusercontent.com/openshift/managed-notifications/master/osd/incident_resolved.json -p ALERT_NAME="alert"

# Post an internal-only service log message
osdctl servicelog post ${CLUSTER_ID} -i -p "MESSAGE=This is an internal message"

# Post a short external message
osdctl servicelog post ${CLUSTER_ID} -r "summary=External Message" -r "description=This is an external message" -r internal_only=False

# Post a service log to a group of clusters, determined by an OCM query
ocm list cluster -p search="cloud_provider.id is 'gcp' and managed='true' and state is 'ready'"
osdctl servicelog post -q "cloud_provider.id is 'gcp' and managed='true' and state is 'ready'" -t file.json
Expand All @@ -76,9 +86,10 @@ func newPostCmd() *cobra.Command {
},
}

// define required flags
// define flags
postCmd.Flags().StringVarP(&opts.Template, "template", "t", "", "Message template file or URL")
postCmd.Flags().StringArrayVarP(&opts.TemplateParams, "param", "p", opts.TemplateParams, "Specify a key-value pair (eg. -p FOO=BAR) to set/override a parameter value in the template.")
postCmd.Flags().StringArrayVarP(&opts.Overrides, "override", "r", opts.Overrides, "Specify a key-value pair (eg. -r FOO=BAR) to replace a JSON key in the document, only supports string fields")
smarthall marked this conversation as resolved.
Show resolved Hide resolved
postCmd.Flags().BoolVarP(&opts.isDryRun, "dry-run", "d", false, "Dry-run - print the service log about to be sent but don't send it.")
postCmd.Flags().StringArrayVarP(&opts.filterParams, "query", "q", []string{}, "Specify a search query (eg. -q \"name like foo\") for a bulk-post to matching clusters.")
postCmd.Flags().BoolVarP(&opts.skipPrompts, "yes", "y", false, "Skips all prompts.")
Expand Down Expand Up @@ -129,15 +140,28 @@ func (o *PostCmdOptions) Run() error {
return err
}

o.parseUserParameters() // parse all the '-p' user flags
o.readFilterFile() // parse the ocm filters in file provided via '-f' flag
o.readTemplate() // parse the given JSON template provided via '-t' flag
o.parseUserParameters() // parse all the '-p' user flags
overrideMap, err := o.parseOverrides() // parse all the '-o' flags
if err != nil {
log.Fatalf("Error parsing overrides: %s", err)
smarthall marked this conversation as resolved.
Show resolved Hide resolved
}

o.readFilterFile() // parse the ocm filters in file provided via '-f' flag
o.readTemplate() // parse the given JSON template provided via '-t' flag

// For every '-p' flag, replace its related placeholder in the template & filterFiles
for k := range userParameterNames {
o.replaceFlags(userParameterNames[k], userParameterValues[k])
}

// Replace any overrides
for overrideKey, overrideValue := range overrideMap {
err := o.overrideField(overrideKey, overrideValue)
if err != nil {
log.Fatalf("could not override '%s': %s", overrideKey, err)
}
}

// Check if there are any remaining placeholders in the template that are not replaced by a parameter,
// excluding '${CLUSTER_UUID}' which will be replaced for each cluster later
o.checkLeftovers([]string{"${CLUSTER_UUID}"})
Expand Down Expand Up @@ -337,6 +361,68 @@ func (o *PostCmdOptions) parseUserParameters() {
}
}

// parseOverides parses all the '-o FOO=BAR' overrides which replace items in the final JSON document
func (o *PostCmdOptions) parseOverrides() (map[string]string, error) {
usageMessageError := errors.New("invalid syntax. Usage: '-r FOO=BAR'")
overrideMap := make(map[string]string)

for _, v := range o.Overrides {
if !strings.Contains(v, "=") {
return nil, usageMessageError
}

param := strings.SplitN(v, "=", 2)
if param[0] == "" || param[1] == "" {
return nil, usageMessageError
}

overrideMap[param[0]] = param[1]
}

return overrideMap, nil
}

func (o *PostCmdOptions) overrideField(overrideKey string, overrideValue string) (err error) {
// Get a pointer, then the value of that pointer so that we can edit the fields
rt := reflect.ValueOf(&o.Message).Elem()

for i := 0; i < rt.NumField(); i++ {
// Get JSON field name
field := rt.Type().Field(i)
jsonName := strings.Split(field.Tag.Get("json"), ",")[0]

if overrideKey == jsonName {
// This shouldn't happen, but if it does we should make a nice error
if !rt.Field(i).CanSet() {
return fmt.Errorf("field cannot be modified")
}

kind := rt.Field(i).Kind()

// Set the field to the overridden value, since we have a string
// we may have to parse it to get the right type
switch kind {
case reflect.String:
rt.Field(i).SetString(overrideValue)

case reflect.Bool:
overrideBool, err := strconv.ParseBool(overrideValue)
if err != nil {
return fmt.Errorf("couldn't parse bool: %s", err)
}
rt.Field(i).SetBool(overrideBool)

default:
return fmt.Errorf("overriding of type %s not implemented", kind)
}

return nil
}
}

return fmt.Errorf("field does not exist")
}

// accessFile returns the contents of a local file or url, and any errors encountered
func (o *PostCmdOptions) accessFile(filePath string) ([]byte, error) {

Expand Down Expand Up @@ -392,6 +478,21 @@ func (o *PostCmdOptions) readTemplate() {
return
}

// If neither `-i` or `-t` is specified, but `-r` is specified at least once then use a pre-canned template
if !o.InternalOnly && (o.Template == "") && (len(o.Overrides) != 0) {
messageTemplate := []byte(`
{
"severity": "Info",
"service_name": "SREManualAction",
"internal_only": true
}
`)
if err := o.parseTemplate(messageTemplate); err != nil {
log.Fatalf("Cannot not parse the default message template.\nError: %q\n", err)
}
return
}

if o.Template == "" {
log.Fatalf("Template file is not provided. Use '-t' to fix this.")
}
Expand Down
110 changes: 110 additions & 0 deletions cmd/servicelog/post_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package servicelog

import (
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/openshift/osdctl/internal/servicelog"
)

func TestSetup(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Setup Suite")
}

var _ = Describe("Test posting service logs", func() {
var options *PostCmdOptions

BeforeEach(func() {
options = &PostCmdOptions{
Overrides: []string{
"description=new description",
"summary=new summary",
},
Message: servicelog.Message{
Summary: "The original summary",
InternalOnly: false,
},
}
})

Context("overriding a field", func() {
It("overrides string fields successfully", func() {
overrideString := "Overridden Summary"
err := options.overrideField("summary", overrideString)

Expect(err).ShouldNot(HaveOccurred())
Expect(options.Message.Summary).To(Equal(overrideString))
})

It("overrides bool fields correctly", func() {
Expect(options.Message.InternalOnly).ToNot(Equal(true))

err := options.overrideField("internal_only", "true")

Expect(err).ShouldNot(HaveOccurred())
Expect(options.Message.InternalOnly).To(Equal(true))
})

It("errors when overriding a field that does not exist", func() {
err := options.overrideField("does_not_exist", "")

Expect(err).Should(HaveOccurred())
})

It("errors when overriding a bool with an unparsable string", func() {
err := options.overrideField("internal_only", "ThisIsNotABool")

Expect(err).Should(HaveOccurred())
})

It("errors when overriding an unsupported data type", func() {
err := options.overrideField("doc_references", "DoesntMatter")

Expect(err).Should(HaveOccurred())
})
})

Context("parsing overrides", func() {
It("parses correctly", func() {
overrideMap, err := options.parseOverrides()

Expect(err).ShouldNot(HaveOccurred())
Expect(overrideMap).To(HaveKey("description"))
Expect(overrideMap["description"]).To(Equal("new description"))
Expect(overrideMap).To(HaveKey("summary"))
Expect(overrideMap["summary"]).To(Equal("new summary"))
})

It("fails when an option contains no equals sign", func() {
options.Overrides = []string{
"THISDOESNOTHAVEANEQUALS",
}

_, err := options.parseOverrides()

Expect(err).Should(HaveOccurred())
})

It("fails when an option has no key", func() {
options.Overrides = []string{
"=VALUE",
}

_, err := options.parseOverrides()

Expect(err).Should(HaveOccurred())
})

It("fails when an option has no value", func() {
options.Overrides = []string{
"KEY=",
}

_, err := options.parseOverrides()

Expect(err).Should(HaveOccurred())
})
})
})