umegaya / lua-aws

pure-lua implementation of aws REST APIs
122 stars 35 forks source link

adopting to new service definition json files #9

Closed umegaya closed 9 years ago

umegaya commented 9 years ago

aws-sdk-js seems to change its service definition json files at v2.x, lua-aws should follow it to get along with new service addition.

CriztianiX commented 9 years ago

Any idea for this? I need use CloudWatchLog and only is available in aws-sdk-js 2.x

umegaya commented 9 years ago

@CriztianiX looking through the new service definition JSON files and new aws-sdk-js code, I will go with *.min.json files. it seems minimum service API definition, and difference from current service definition is some common data structure among API, is shared (its called shape in aws-sdk-js code). so migration from 1.x service definition seems not to be so difficult.

OTOH, there is other service definitions like .normal.json, .waiter.json, *.paginator.json. I think corresponding to these defs brings unnecessary complexity to lib, so I will not do it.

I will try to correspond to *.min.json service definition in this week.

CriztianiX commented 9 years ago

Yes! Looking in the new definations, now they have "metadata" object:

metadata": { "apiVersion": "2015-04-15", "endpointPrefix": "ec2", "serviceAbbreviation": "Amazon EC2", "serviceFullName": "Amazon Elastic Compute Cloud", "signatureVersion": "v4", "xmlNamespace": "http://ec2.amazonaws.com/doc/2015-04-15", "protocol": "ec2" }

the only fields than non-exists more from v1 is: "format".

CriztianiX commented 9 years ago

Hey, I'am starting to port it to version 2.0. Take a look at this branch: https://github.com/CriztianiX/lua-aws/tree/2.0 Now, i'am testing with ec2 service, and its working "describeInstances"! This line "https://github.com/CriztianiX/lua-aws/blob/2.0/lua-aws/api.lua#L63" need to be change!, but I think it's a good starting point

What do yo think? Saludos!

umegaya commented 9 years ago

@CriztianiX hi, thank you for experiment. glad to hear that these small fix can work with some of ec2 API correctly. if following points are improved, I think it can be merged into master.

  1. fix all TODOs and pass all test.
  2. for API name, please keep backward compatibility. for example, user can use both describeInstance and DescribeInstance to call ec2 describeInstance API.
  3. please keep "endpoint" and "prefix" auto-detection.

if these are too much work for you, please wait for my work. then I will use your work as reference.

umegaya commented 9 years ago

@CriztianiX hi, I've done v2.0 work and create PR #17. can you check cloudwatchlog works and no regression happen? thanks.

CriztianiX commented 9 years ago

Hey! I've merged PR#17 in my "2.0" branch. (you can see it: https://github.com/CriztianiX/lua-aws/commits/2.0) All it is works fine! Will we keep branches for 1.0 and 2.0 or will we merge to master branch?

umegaya commented 9 years ago

@CriztianiX thank you for checking :D I will create 1.0 branch with current master, and merge #17 into master. so master will be 2.0

CriztianiX commented 9 years ago

Good! The only diference betwen your branch and mine is https://github.com/CriztianiX/lua-aws/commit/04c4b11eba2830bc7b83809a77167a378aef95a1 Any suggest why you want keep this functions in api class?

umegaya commented 9 years ago

hi, because I think these functions is not suitable for exposing for common use, by following reason.

  1. any other part of code will not want to use get_XXX_from_env. (see also: YAGNI principle)
  2. current error message when environment variable not set, is actually AWS_API's use case specific, also throwing error is not good behavior if it is exposed as util (I think it better returning nil in such case)

so I want to keep these functions private (for now).

but this is rather my flavor than actual benefit, if following fix is added, it can be merged into master. A. get_XXX_from_env return nil when env is not set. B. inside class.AWS_API, throwing error when config not set and get_XXX_from_env returns nil

umegaya commented 9 years ago

I close this issue. for fixing get_XXX_from_env, please create another issue or PR.