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

Fix path with contained query #3321

Merged
merged 1 commit into from Dec 11, 2014

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Dec 9, 2014

Query inside path is not allowed, it should be under
separate key :query.

Putting query inside path leads to error:

URI::InvalidComponentError Exception: bad
component(expected absolute path component):
/v1.1/images/detail?limit=20

Caused most probably by this comparison:
URI::parser.regexp[:ABS_PATH] !~ v

@geemus
Copy link
Member

geemus commented Dec 10, 2014

Query can be a hash also, actually, and I think in this case it might be clearer to do so. I'll include examples inline for clarity. What do you think? Thanks!

if attribute
path = "images/detail?#{attribute}=#{URI::encode(query)}"
query = "#{attribute}=#{URI::encode(query)}"
Copy link
Member

Choose a reason for hiding this comment

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

So this might be { attribute => URI::encode(query) }.

@Ladas
Copy link
Contributor Author

Ladas commented Dec 11, 2014

I misunderstood on first read through.
Seems like we should rather allow any query here and change the interface. Passing only one query is limiting. But I am not sure what is your backwards compatibility policy when you change interface.

I'll test the hash syntax, seems nicer.

Query inside path is not allowed, it should be under
separate key :query.

Putting query inside path leads to error:

URI::InvalidComponentError Exception: bad
component(expected absolute path component):
/v1.1/images/detail?limit=20

Caused most probably by this comparison:
URI::parser.regexp[:ABS_PATH] !~ v
@Ladas Ladas force-pushed the openstack_fix_list_public_image_detailed branch from c89c909 to cc04c7f Compare December 11, 2014 09:11
@Ladas
Copy link
Contributor Author

Ladas commented Dec 11, 2014

Cool, seems like the hash syntax works. Thanks for pointing that out.

@geemus
Copy link
Member

geemus commented Dec 11, 2014

Looks good, thanks!

geemus added a commit that referenced this pull request Dec 11, 2014
@geemus geemus merged commit 7ab36e6 into fog:master Dec 11, 2014
Ladas added a commit to Ladas/manageiq that referenced this pull request Dec 16, 2014
Pagination is always forced by Glance conf file. So we
need to always obtain all pages of images in cycle.

Depends-On: fog/fog#3321
Depends-On: fog/fog#3322

https://bugzilla.redhat.com/show_bug.cgi?id=1164764
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.

None yet

2 participants