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

Add Encoding & File to YAML template #338

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

ajwans
Copy link

@ajwans ajwans commented Apr 29, 2021

Many of the options to yab can be specified in a YAML template. In my case I want to specify a file to read the request data from and also to specify how the request is encoded.

This diff enables these options within the template file.

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2021

CLA assistant check
All committers have signed the CLA.

@Dogild
Copy link
Contributor

Dogild commented Apr 30, 2021

Thanks for the contribution, is there any chance to update the summary of the PR to get more context on this please?

@ajwans
Copy link
Author

ajwans commented Apr 30, 2021

I updated the top comment, is that what you need?

Copy link
Contributor

@Dogild Dogild left a comment

Choose a reason for hiding this comment

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

Please update the summary of the diff with examples of this new feature. Can you also please update the Changelog with this new feature

case "json":
opts.ROpts.Encoding = encoding.JSON
case "protobuf":
opts.ROpts.Encoding = encoding.Protobuf
Copy link
Contributor

Choose a reason for hiding this comment

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

When using the option -e, --encoding, encoding.Protobuf will match with proto and not protobuf.

return err
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should prevent users of using file and request together. Right now, File have priority over request, only one should be available

case "raw":
opts.ROpts.Encoding = encoding.Raw
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 169, there is the following line of code overrideParam(&opts.ROpts.RequestJSON, string(body)). How would it work when the body given in file has another format than JSON? Right now, body could be with format proto, thrift or raw.

opts := newOptions()
err := readYAMLFile("testdata/valid.yab", nil, opts)
assert.NoError(t, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more test please? We should provide the following unit tests:

  • Yab template with file + JSON format
  • Yab template with file + YAML format
  • Yab template with file + RAW format
  • Yab template with file + Proto format
  • Yab template with file + Thrift format
  • Negative test with bad combinaison of options (like File and request together)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants