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

Add nova shared_instance_storage support #255

Merged
merged 1 commit into from
Mar 3, 2015
Merged

Add nova shared_instance_storage support #255

merged 1 commit into from
Mar 3, 2015

Conversation

rsalevsky
Copy link
Contributor

No description provided.

@MaximilianMeister
Copy link
Contributor

we should document those options somewhere.
nova_with_ssl and maybe more options are also not documented anywhere, not even in the undocumented options.
opened #256

but codewise 👍 as it has been done like this for nova_with_ssl as well

dguitarbite added a commit that referenced this pull request Mar 3, 2015
Add nova shared_instance_storage support
@dguitarbite dguitarbite merged commit f9aff32 into SUSE-Cloud:master Mar 3, 2015
@aspiers
Copy link
Contributor

aspiers commented Mar 4, 2015

we should document those options somewhere.

Right. A good rule of thumb is: no PR should ever increase the amount of technical debt. So please do not ever add new options without adding the corresponding documentation. If the new options are similar to already existing options which are missing documentation, then that is not an excuse to skip documenting the new options; rather it is a good opportunity to add documentation for the existing options alongside the new documentation for the new options :)

So personally I would have said -1, but thanks to Max for submitting #256 so we can at least ensure this gets fixed later.

@dguitarbite
Copy link
Contributor

I did not see a -1 from Max, so I assumed that its more of a general suggestion than a no go for this patch. I support documenting the features. Thanks for creating #256.

@rsalevsky
Copy link
Contributor Author

I discussed this with Max yesterday, the point is we have no suitable documentation for the options. There is only a pull request #228 which introduce the documentation.

@MaximilianMeister
Copy link
Contributor

There is only a pull request #228 which introduce the documentation.

not for those undocumented options, I just took the documentation we already have

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