Skip to content
This repository has been archived by the owner on Mar 19, 2018. It is now read-only.

Add functionality #27

Merged
merged 13 commits into from
Dec 6, 2015
Merged

Conversation

shortdudey123
Copy link
Contributor

  • Update Ruby syntax with rubocop
  • Add host create, update, and delete

@kris-lab
Copy link
Contributor

@shortdudey123 thanks for your contribution. I will have a look asap!

@shortdudey123
Copy link
Contributor Author

@kris-lab no problem! Let me know if you have any questions. (I also tried to do some clean up as well as add hosts functionality)

@kris-lab
Copy link
Contributor

@shortdudey123 sorry it takes so long but I am bit overloaded currently.

Three main things:

  • .travis.yml: I would keep also 1.9.3 - this is my use case on the production datacenter. We use debian/wheezy. We run mms-api e.g. inside pulsar-rest-api tasks to automate health check of mongo cluster
  • can we add some spec for host resource? I know it is a bit fake but still would be nice to mock it?
  • there are some changes for resource abstract with set_client/set_data: we should bump a version but in this case maybe we should jump to the 0.2.0? wdyt?

again, many thanks for your effort!

@shortdudey123
Copy link
Contributor Author

.travis.yml: I would keep also 1.9.3

You should really upgrade :p

can we add some spec for host resource

Which part? lib/mms/resource/host.rb?

we should bump a version

Definitely agree on the rev since its breaking changes. I normally leave the version changes up to the author, however, i can bump it to 0.2.0 if you wish

@kris-lab
Copy link
Contributor

You should really upgrade :p

we are working on upgrade but it will take a while so it is important at the moment to keep 1.9.3 :/

Which part? lib/mms/resource/host.rb?

yes, only this lib/mms/resource/host.rb?

we should bump a version

well, agree but in this case you can bump it and if PR is good we can merge and release!

@shortdudey123
Copy link
Contributor Author

@kris-lab updated per your feedback, let me know if there is anything else i should address

@kris-lab
Copy link
Contributor

Thanks! I will have a look and get back to you asap...

@shortdudey123
Copy link
Contributor Author

let me know if there is anything else you need from my end :)

@@ -79,8 +143,8 @@ def restorejobs
def restorejob_create(type_value, group_id, cluster_id)
if type_value.length == 24
find_group(group_id).cluster(cluster_id).snapshot(type_value).create_restorejob
elsif datetime = (type_value == 'now' ? DateTime.now : DateTime.parse(type_value))
raise('Invalid datetime. Correct `YYYY-MM-RRTH:m:sZ`') if datetime.nil?
elsif datetime == (type_value == 'now' ? DateTime.now : DateTime.parse(type_value))
Copy link
Contributor

Choose a reason for hiding this comment

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

@shortdudey123 I think this is a bug. I see that rubocop suggest to use == but the bug is there from the beginning:)! We should use else instead of elsif otherwise datetime will be lost...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay for fixing bugs :p

Something like this?

else
  datetime = (type_value == 'now' ? DateTime.now : DateTime.parse(type_value))

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, exactly like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kris-lab
Copy link
Contributor

kris-lab commented Dec 6, 2015

lvgtm. I am going to merge it now. I will do more tests before final release. Many thanks!

kris-lab pushed a commit that referenced this pull request Dec 6, 2015
@kris-lab kris-lab merged commit 9600c12 into cargomedia:master Dec 6, 2015
@kris-lab kris-lab mentioned this pull request Dec 6, 2015
2 tasks
@shortdudey123 shortdudey123 deleted the add_functionality branch December 6, 2015 09:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants