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

Describe service #209

Open
wants to merge 4 commits into
base: 2.x.x
Choose a base branch
from
Open

Describe service #209

wants to merge 4 commits into from

Conversation

rahulrayal
Copy link
Contributor

Odin version(x.y.z)

Features

Resources

if labelsJSON != "" {
err := json.Unmarshal([]byte(labelsJSON), &labels)
if err != nil {
log.Fatal("Error parsing JSON, the the key and values should be strings: ", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the input is not a valid json, then will this error message will be misleading?


var serviceName string
var serviceVersion string
var labelsJSON string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use camel case consistently?

optional google.protobuf.Struct labels = 7;
optional google.protobuf.Struct service_definition = 8;
repeated google.protobuf.Struct provisioning_config_files = 9;
repeated string versions = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have both version and versions?

}

message DescribeServiceRequest {
string service_name = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be consistent in naming these eg. either have name and version OR service_name and service_version?

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

Successfully merging this pull request may close these issues.

2 participants