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

(#116): Remove mandatory parameter checking for namespace in /lookup #117

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

crayfishx
Copy link
Member

@crayfishx crayfishx commented Feb 12, 2018

As discussed in #116 - namespace has been made optional in other areas (CLI...etc) so the API should not enforce this parameter.

This removes the mandatory parameter check for the namespace parameter in API calls to /lookup/

@crayfishx crayfishx changed the title (#11): Remove mandatory parameter checking for namespace in /lookup (#116): Remove mandatory parameter checking for namespace in /lookup Feb 12, 2018
}

if params['namespace']
request_opts[:namespace] = params['namespace'].split(/\//),

Choose a reason for hiding this comment

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

Unless I'm mistaken, the , at the end is a syntax error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh! Must add basic syntax to the rake task for testing!

@jtopjian
Copy link

@crayfishx What would be an example of doing a namespace-less lookup using the test fixtures?

@crayfishx
Copy link
Member Author

@jtopjian I'll add an example on this PR, when I fix the syntax error :)

@crayfishx
Copy link
Member Author

@jtopjian The file resource will simply operate one level up when there is no namespace defined.... so <path>/<namespace>.yaml becomes <path>.yaml - I've added an example in crayfishx@f4b1e23 - in keeping with random data sets, I've chosen biscuits

@beddari
Copy link
Member

beddari commented Feb 14, 2018

Those new fixtures are useful 👍

@jtopjian
Copy link

@crayfishx Awesome - thank you :)

I've made the appropriate changes here: jerakia/go-jerakia#1. Once this PR is merged, I'll merge into the go library.

@crayfishx
Copy link
Member Author

I think we can push this out as a patch release - the expected behaviour was that namespace is optional so the mandatory check not being removed was an oversight... i'll roll this into a 2.5.1 release so it's compatible with the library you're writing.

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

Successfully merging this pull request may close these issues.

3 participants