-
Notifications
You must be signed in to change notification settings - Fork 74
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
Adding Action Annotations #631
Conversation
reader.bindPackageInputsAndAnnotations() | ||
reader.bindActionInputsAndAnnotations() | ||
reader.bindTriggerInputsAndAnnotations() | ||
if err := reader.bindPackageInputsAndAnnotations(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for surfacing these errors from the "bind" functions.
deployers/deploymentreader.go
Outdated
} | ||
} | ||
if !keyExistsInManifest { | ||
return utils.NewYAMLFormatError("Annotation key \""+name+"\" does not exist in manifest file but specified in deployment file.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can move this string to our translatable file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
deployers/deploymentreader.go
Outdated
} | ||
|
||
serviceDeployPack.Package.Annotations = keyValArr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the array always allocated now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if one or more annotations are specified ...
case "aaa": | ||
assert.Equal(t, "this is an annotation", annos.Value, "Failed to get action annotations") | ||
case "action_annotation_1": | ||
assert.Equal(t, "this is a action annotation 1", annos.Value, "Failed to get action annotations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c /a/an/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
case "action_annotation_1": | ||
assert.Equal(t, "this is a action annotation 1", annos.Value, "Failed to get action annotations") | ||
case "action_annotation_2": | ||
assert.Equal(t, "this is a action annotation 2", annos.Value, "Failed to get action annotations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c /a/an/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
for _, annos := range wskaction.Annotations { | ||
switch annos.Key { | ||
case "action_annotation_1": | ||
assert.Equal(t, "this is a action annotation 1 from deployment", annos.Value, "Failed to get action annotations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c /a/an/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
case "action_annotation_1": | ||
assert.Equal(t, "this is a action annotation 1 from deployment", annos.Value, "Failed to get action annotations") | ||
case "action_annotation_2": | ||
assert.Equal(t, "this is a action annotation 2", annos.Value, "Failed to get action annotations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c /a/an/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
type: string | ||
description: a simple greeting message, Hello World! | ||
annotations: | ||
action_annotation_1: this is annotation 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test for more complex (i.e., arrays/JSON) annotations? Should we open another issue (feature) to support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, created new issue #634
Closes #609
Please don't get scared with the number of files changed :) this number includes YAML files for unit tests and updates to YAML files of existing integration test and a new integration test directory.
Here is the list of changes:
misc.go
intowebaction.go
web-export
wskdeploy
errors out sayingError: wskdeploy.go [125]: [ERROR_COMMAND_FAILED]: wskdeploy: Error: deploymentreader.go [227]: [ERROR_YAML_FORMAT_ERROR]: Annotation key <key> does not exist in manifest file but specified in deployment file.
wskdeploy
overwrites annotation value from deployment i.e. annotation value in deployment file takes higher precedence over the value in manifest file.