rackspace / gophercloud

A Go SDK for OpenStack. IN FEATURE FREEZE. See Issue #592
http://gophercloud.io
Other
456 stars 181 forks source link

[RFR] Rackspace Auto Scale: policies #554

Closed bison closed 8 years ago

bison commented 8 years ago

Adds a package for Rackspace Auto Scale policies.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 80.179% when pulling 4d1a9a6c797895b198b3365a4c82fb52584764ae on bison:autoscale-policies into a09b5b4eb58195b6fb3898496586b8d6aeb558e0 on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 80.207% when pulling 92d20ec52cbfb6db86aeabfe1b08af7317c1ed8c on bison:autoscale-policies into a09b5b4eb58195b6fb3898496586b8d6aeb558e0 on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 80.19% when pulling eedc8d932fcb100b9957c640f9f775759f7d8a5a on bison:autoscale-policies into a09b5b4eb58195b6fb3898496586b8d6aeb558e0 on rackspace:master.

jrperritt commented 8 years ago

Looks great. Let me know when you're done making changes and I'll merge it.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.06%) to 80.127% when pulling 0310e5c0dde7db5cea9d7ed24c3030f51ac9fdb7 on bison:autoscale-policies into a09b5b4eb58195b6fb3898496586b8d6aeb558e0 on rackspace:master.

bison commented 8 years ago

Pushed 0310e5c with the ScheduleArgs interface. It's not finished. I haven't changed the create and update bits, but this works for getting / listing policies. Switching on the schedule type in those cases would look like this:

policy, err := policies.Get(client, group, policyID).Extract()

switch s := policy.Schedule.(type) {
case policies.At:
        t := time.Time(s)
        fmt.Println("EXECUTE AT:", t.Format(time.RFC850))

case policies.Cron:
        fmt.Println("CRON:", s)

default:
        fmt.Println("Webhook.")
}

Creating policies with an At schedule seems nice with this because you can use all the standard time manipulation stuff and then create the At from that. Users shouldn't have to worry about the ToPolicyArgs() method. The methods for converting to maps for creates and updates can handle that.

bison commented 8 years ago

Added the ScheduleArgs bits for create and update operations. Examples in the tests.

I think this is finished assuming you're good with the ScheduleArgs thing. If not, let me know and we can work out another way.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 80.177% when pulling a06e2ca451592d0e191ff91dc64586886fe6605e on bison:autoscale-policies into a09b5b4eb58195b6fb3898496586b8d6aeb558e0 on rackspace:master.

jrperritt commented 8 years ago

Maybe we can compromise and just have AtSchedule and CronSchedule fields? After thinking about it, I don't think we should force users to switch (neither on type nor value) to figure out what's there.

bison commented 8 years ago

We can do that. My only worry is the zero values. I really think AtSchedule works best as a time.Time, but if I request a policy configured with a cron schedule, I don't want AtSchedule set to 0001-01-01 00:00:00 +0000 UTC... and in the opposite case, even having Cron be an empty string is a little strange.

We could make those fields interface{}, so they can default to nil, but that's not great either and it seems like users will have to do some checking to see what's what in any case.

bison commented 8 years ago

Or did you mean keep the ScheduleArgs interface, but just have two fields and do some type validation before creates / updates?

jrperritt commented 8 years ago

I'm very much a fan of the ScheduleArgs in the CreateOpts and UpdateOpts; I think that works well. I'm trying to figure out if there's a way for users to get at that information in a response object without having to type-assert. Though I agree zero-values may not be the best solution either.

bison commented 8 years ago

We could keep the ScheduleArgs interface and the two types for create and updates, and revert to Args map[string]string for Policy. If you request a policy with an at argument, then need to update it, you can always use time.Parse to turn it into a policies.At.

That doesn't seem ideal, and you still have to do some looking into the map, but maybe it's more straight forward than the type switch. I'm not really sure.

jrperritt commented 8 years ago

No, let's go with what you have now. I think it's the best of the options I can think of right now.

jrperritt commented 8 years ago

Is it ready to merge?

bison commented 8 years ago

Yeah, I think it's good.

jrperritt commented 8 years ago

+2