Skip to content

Bluemix object storage fixes#25

Merged
Ladas merged 1 commit intofog:masterfrom
gekola:allow-omitting-auth-port
Mar 4, 2016
Merged

Bluemix object storage fixes#25
Ladas merged 1 commit intofog:masterfrom
gekola:allow-omitting-auth-port

Conversation

@gekola
Copy link
Copy Markdown
Contributor

@gekola gekola commented Mar 2, 2016

  • Fix 500 due to port presence in Host header for bluemix object storage
  • Recognize userid as argument

This is #1 with making port omitting optional.

@seanhandley
Copy link
Copy Markdown
Member

Thanks @gekola. I'm ok with this, though I think you should probably pass that option through to the Keystone v2 method as well.

@gekola gekola force-pushed the allow-omitting-auth-port branch from e56f27f to ce4c446 Compare March 2, 2016 14:10
@gekola
Copy link
Copy Markdown
Contributor Author

gekola commented Mar 2, 2016

Thanks for the feedback, @seanhandley. I updated the PR to also apply port omitting option to the Keystone v2 method.

@seanhandley
Copy link
Copy Markdown
Member

Ok great. And what's the motivation for putting the userid attribute into the storage class?

@seanhandley
Copy link
Copy Markdown
Member

I ask because I'm not sure why it isn't present in the compute, image, network etc

@gekola
Copy link
Copy Markdown
Contributor Author

gekola commented Mar 2, 2016

@seanhandley while working with Carrierwave and Paperclip there is a warning [fog][WARNING] Unrecognized arguments: openstack_userid on each file uploading/downloading. Adding userid to parameters recognized by storage removes this warning (I figured since domain and username are there it is OK to put userid there also). I am new to fog, so I am not sure why it is not there for compute, image and so on. But I guess it was not added to them when Keystone v3 support was added while it should have been.

@seanhandley
Copy link
Copy Markdown
Member

Ok thanks @gekola - Thumbs up from me 👍

We'll maybe look at re-architecting the gem a little so the parameters aren't duplicated between OpenStack projects.

Comment thread lib/fog/openstack.rb
tenant_name = options[:openstack_tenant].to_s
auth_token = options[:openstack_auth_token] || options[:unscoped_token]
uri = options[:openstack_auth_uri]
omit_default_port = options[:openstack_auth_omit_default_port]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

indentation nit, we should have rubocop soon, could you indent all = the same? Applies to all below

@seanhandley hm, what are we missing to get rubocop running for each PR?

@Ladas
Copy link
Copy Markdown
Contributor

Ladas commented Mar 4, 2016

@gekola just last comments, then I am ready to merge :-)

 * Fix 500 due to port in Host header for bluemix object storage
 * Recognize userid as argument
@gekola gekola force-pushed the allow-omitting-auth-port branch 2 times, most recently from 71f7494 to e634787 Compare March 4, 2016 09:27
@seanhandley
Copy link
Copy Markdown
Member

@Ladas You need to run rubocop from Travis on each build afaik but I've never done it myself (I use a custom integration with RubyCritic and Slack for my commercial projects).

From the looks, you can reference rubocop in the .travis.yml and then pass it arguments to configure how strict you need it to be.

@gekola
Copy link
Copy Markdown
Contributor Author

gekola commented Mar 4, 2016

@seanhandley @Ladas you can see an example of using rubocop with travis in rubocop repository. I do not think adding rubocop check to CI it is a good idea at this point as there are too many offenses in the code, so the build would always fail.

Ladas added a commit that referenced this pull request Mar 4, 2016
@Ladas Ladas merged commit 70c9d43 into fog:master Mar 4, 2016
@Ladas
Copy link
Copy Markdown
Contributor

Ladas commented Mar 4, 2016

@gekola awesome, thank you :-)

@gekola @seanhandley btw. for ManageIQ the rubocop runs only on the lines changed by the PR, so we can incrementally make it better. I will ask the guys how to setup that.

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.

3 participants