Support Cloud Monitoring #99
Conversation
vkcs/provider.go
Outdated
|
|
||
| var tempUrl = map[string]string{ | ||
| "alerting": "https://mcs.mail.ru/infra/alerting/v1/", | ||
| "templater": "https://mcs.mail.ru/infra/templater/v2/", |
There was a problem hiding this comment.
any chance to put it in keystone catalog?
vkcs/provider.go
Outdated
|
|
||
| var tempUrl = map[string]string{ | ||
| "alerting": "https://mcs.mail.ru/infra/alerting/v1/", | ||
| "templater": "https://mcs.mail.ru/infra/templater/v2/", |
There was a problem hiding this comment.
version number should not be a part of url settings
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| Description: "Type of channel: email or sms.", |
There was a problem hiding this comment.
Perhaps add ValidateFunc to check that only "email" or "sms" is specified?
| } | ||
| d.SetId(ch.Channel.ID) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
We should return resourceChannelRead(ctx, d, meta) in create and update methods
| return diag.Errorf("Error update VKCS monitoring channel: %s", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
We should return resourceChannelRead(ctx, d, meta) in create and update methods
| return diag.Errorf("Error creating VKCS monitoring template: %s", err) | ||
| } | ||
| d.SetId(t.LinkID) | ||
| d.Set("script", t.Script) |
There was a problem hiding this comment.
We should set computed (and other fields except id) in resourceTemplaterRead method
| return &schema.Resource{ | ||
| CreateContext: resourceTemplaterCreate, | ||
| ReadContext: resourceTemplaterRead, | ||
| UpdateContext: resourceTemplaterUpdate, |
There was a problem hiding this comment.
If there is no fields that we can update, then we may remove this method
|
|
||
| err = channelDelete(MonitoringV1Client, config.GetTenantID(), d.Id()).extractErr() | ||
| if err != nil { | ||
| return diag.Errorf("Error de VKCS monitoring channel: %s", err) |
|
|
||
| err = triggerDelete(MonitoringV1Client, config.GetTenantID(), d.Id()).extractErr() | ||
| if err != nil { | ||
| return diag.Errorf("Error de VKCS monitoring trigger: %s", err) |
| } | ||
| _, err = channelUpdate(MonitoringV1Client, config.GetTenantID(), d.Id(), &chn).extract() | ||
| if err != nil { | ||
| return diag.Errorf("Error update VKCS monitoring channel: %s", err) |
| } | ||
| ch, err := channelGet(MonitoringV1Client, config.GetTenantID(), d.Id()).extract() | ||
| if err != nil { | ||
| return diag.Errorf("Error get VKCS monitoring channel(%s): %s", d.Id(), err) |
| }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Why is order reversed? We usually define tests at the top and constants at the bottom of the file. Although it's disputable which way is better, I believe that we should stick to common style for now.
| notification_title = "123" | ||
| notification_channels = [vkcs_monitoring_channel.basic.id] | ||
| } | ||
| ` |
| }, | ||
| }, | ||
| }) | ||
| } |
|
|
||
| func createChannelURL(c ContainerClient, pid string) string { | ||
| return c.ServiceURL(pid, "notification_channels") | ||
| } |
There was a problem hiding this comment.
Custom urls are usually defined in urls.go
|
|
||
| func (r deleteChannelResult) extractErr() error { | ||
| return r.Err | ||
| } |
There was a problem hiding this comment.
@ftersin @schirevko
In public DNS I decided to use capitalized names for result methods, because it's more convenient and it would be easier to move client logic into separate package, which I believe should be done. If we stick to this way, you could just use gophercloud.ErrResult for deleteChannelResult.
| return r | ||
| } | ||
|
|
||
| // triggerGet performs request to get trigger instance |
| return | ||
| } | ||
|
|
||
| // triggerDelete performs request to delete trigger |
| r.Header = result.Header | ||
| } | ||
| return r | ||
| } |
There was a problem hiding this comment.
Why don't stick to common way with returning (r commonTemplateResult)?
| "channel_type": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
If we are able to update channel_type/address then we should remove ForceNew
Supported: