search

serverless / serverless-kubeless

This plugin enables support for Kubeless within the Serverless Framework.
Apache License 2.0
303 stars 80 forks source link

escapeName helper method deals correctly with camelcase names #163

Closed dimagoldin closed 5 years ago

dimagoldin commented 5 years ago

this fixes #162

the trigger name will be correctly escaped and changed to a dash separated lowercase name to support deployment of functions that relay on camelCase named kafka topics

dimagoldin commented 5 years ago

@andresmgot Could you please look at this PR and merge if its acceptable? :)

later i'll try to find the time to add support for "name" property in the events.trigger part of the serverless.yaml (besides queue and topic)

andresmgot commented 5 years ago

Hi @dimagoldin, I think that a better solution would be to actually modify the two helpers.escapeName and replace them with the lodash function for that purpose: _.kebabCase. That way we don't need to test the functionality ourselves.

Also please bump the version of the package in the package.json. Since this may be a breaking change please bump the minor version: 0.7.0

dimagoldin commented 5 years ago

Hi @dimagoldin, I think that a better solution would be to actually modify the two helpers.escapeName and replace them with the lodash function for that purpose: _.kebabCase. That way we don't need to test the functionality ourselves.

Also please bump the version of the package in the package.json. Since this may be a breaking change please bump the minor version: 0.7.0

I agree, but wouldn't this remove the "escaping" logic all together? is it not needed?

also currently i think the trigger naming logic is inconsistent, the tests use camelCase function name "myFunction" which is expected to stay that way for the both the function name and the trigger name, even though trigger name doesnt support camelCase naming, which this PR is trying to resolve, at least for the topic name part.

@andresmgot Are the changes in the latest commit according to what you wanted?

andresmgot commented 5 years ago

I agree, but wouldn't this remove the "escaping" logic all together? is it not needed?

It's needed, but kebabCase is already doing the scaping that the helper function was doing before.

also currently i think the trigger naming logic is inconsistent, the tests use camelCase function name "myFunction" which is expected to stay that way for the both the function name and the trigger name, even though trigger name doesnt support camelCase naming, which this PR is trying to resolve, at least for the topic name part.

I think I am not following, myFunction shouldn't be a valid name and should be translated to my-function. If the tests are using that as an example it may be wrong.

dimagoldin commented 5 years ago

@andresmgot The changes are ready to be merged, unless you think something needs to change?