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.

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

Jump to bottom

[Bug] d2g subcommand changes done to add support for YAML file. #6765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Bug] d2g subcommand changes done to add support for YAML file. #6765

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
667c33b
d2g subcommand changes done to add support for YAML file.
anirudh-pandey Jan 8, 2024
c0024cd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 38 additions & 5 deletions clients/client-shell/cmds/d2g/d2g.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/xeipuuv/gojsonschema"

"github.com/spf13/cobra"
"sigs.k8s.io/yaml"
)

func init() {
Expand All @@ -37,8 +38,14 @@ To convert a task definition (JSON), you must use the task definition flag (-t,
func convert(cmd *cobra.Command, args []string) (err error) {
isTaskDef, _ := cmd.Flags().GetBool("task-def")
filePath, _ := cmd.Flags().GetString("file")
// Default file extension is json(temporary, handled for piped input)
fileExtension := "json"

input, err := userInput(filePath)
// Read file extension(temporary, handled for piped input)
if len(filePath) >= 4 {
fileExtension = filePath[len(filePath)-4:]
}
input, err := userInput(filePath, fileExtension)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than checking the file extension, it would be better to check the content. There are many variations for the file extension, including, but not limited to no extension, .yaml, .yml, .YAML, .YML, .json, .JSON. What matters really is only whether the content can be successfully interpreted as a particular format, and if it passes the payload validation (i.e. includes required fields, etc). For this, it is only necessary to attempt to parse the content in a particular format, and then decide based on whether the data was successfully parsed, whether the data is of the particular format you are trying.

if err != nil {
return err
}
Expand Down Expand Up @@ -95,16 +102,23 @@ func convert(cmd *cobra.Command, args []string) (err error) {
return fmt.Errorf("failed to unmarshal generic worker task definition: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be changed. If you aren't able to unmarshal, it suggests the input format is not json. At this point, knowing it is not json, the next task is to see if it is yaml.

}

gwPayloadJSON, err := json.Marshal(gwTaskDef["payload"])
gwPayload, err := json.Marshal(gwTaskDef["payload"])
if err != nil {
return fmt.Errorf("failed to marshal generic worker payload: %v", err)
}
err = validateJSON(gwPayloadJSON, genericworker.JSONSchema())
err = validateJSON(gwPayload, genericworker.JSONSchema())
if err != nil {
return fmt.Errorf("output validation failed: %v", err)
}

fmt.Fprintln(cmd.OutOrStdout(), string(gwTaskDefJSON))
// Convert to original extension given by user
if isYAMLExtension(fileExtension) {
gwPayload, err = yaml.JSONToYAML(gwPayload)
if err != nil {
return fmt.Errorf("failed to convert from JSON To YAML: %v", err)
}
}
fmt.Fprintln(cmd.OutOrStdout(), string(gwPayload))
} else {
// Convert dwPayload to gwPayload
gwPayload, err := d2g.Convert(dwPayload)
Expand All @@ -124,16 +138,30 @@ func convert(cmd *cobra.Command, args []string) (err error) {
return fmt.Errorf("output validation failed: %v", err)
}

// Convert to original extension given by user
if isYAMLExtension(fileExtension) {
formattedActualGWPayload, err = yaml.JSONToYAML(formattedActualGWPayload)
if err != nil {
return fmt.Errorf("failed to convert from JSON To YAML: %v", err)
}
}
fmt.Fprintln(cmd.OutOrStdout(), string(formattedActualGWPayload))
}

return nil
}

func userInput(filePath string) (input json.RawMessage, err error) {
func userInput(filePath string, fileExtension string) (input json.RawMessage, err error) {
if filePath != "" {
// Read input from file
input, err = os.ReadFile(filePath)
// Check if input is in yaml, then convert to json
if isYAMLExtension(fileExtension) {
input, err = yaml.YAMLToJSON(input)
if err != nil {
return nil, fmt.Errorf("failed to convert YAML To JSON: %v", err)
}
}
if err != nil {
return nil, fmt.Errorf("failed to read input file: %v", err)
}
Expand Down Expand Up @@ -179,3 +207,8 @@ func validateJSON(input []byte, schema string) error {

return nil
}

func isYAMLExtension(fileExtension string) bool {
Copy link
Member

@petemoore petemoore Jan 24, 2024 •
edited

Choose a reason for hiding this comment

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

As mentioned above, this is not the best way to determine the format of the content. Furthermore, this function would consider a file called foo.notyaml to be a yaml file. The standard library provides filepath.Ext for this purpose which handles variable length extensions, but we shouldn't need to consider the filename, especially because the input can come from standard in, so we need a means to be able to deduce it without a filename.

fileExtensionLower := strings.ToLower(fileExtension)
return fileExtensionLower == "yaml" || fileExtensionLower == ".yml"
}