Skip to content

Update ssm.cfc#53

Open
deanmaunder wants to merge 3 commits intojcberquist:masterfrom
deanmaunder:ssm-getparamsbypath-putparam
Open

Update ssm.cfc#53
deanmaunder wants to merge 3 commits intojcberquist:masterfrom
deanmaunder:ssm-getparamsbypath-putparam

Conversation

@deanmaunder
Copy link

add getParametersByPath
add putParameter

@jcberquist
Copy link
Owner

Thank you for the pull request, and sorry for the delay in getting to it. In looking at what you have here, I have a few questions and notes. First of all, it doesn't look to me like all of the possible parameters that can be used in these methods are there. For example, you don't have the ParameterFilters option in the getParametersByPath method. I am not opposed necessarily to leaving out some of the possible options, since someone else can add them later if they need them, but it would be nice to get them in.

Also, I see that you automatically keep make http requests in this method when there are enough results to require pagination. I would prefer to just wrap the API call as is, and then one can implement pagination in their use of the API rather than having it done automatically here. There might be a place in this library for automatic paginators, but I don't want to do it in the raw API method wrappers.

I would further like to always return the full API response struct instead of just the data itself, so that users can get at that data if they need it.

Thanks!

@deanmaunder
Copy link
Author

Thanks for the notes, I really did this to fit a local use case and thought it might be useful to others, I'll have a run through your comments next time Im updating the library and push up the changes.
Cheers!

@chapmandu
Copy link

Bump on this one.. I was going to write it myself but you beat me to it. This would be a great addition. Nice work.

@tomchiverton
Copy link

I'd rather have a partial impl. that can be extended, than none.

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.

4 participants