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

Support Cloud Monitoring #99

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support Cloud Monitoring #99

wants to merge 6 commits into from

Conversation

vi4eslav
Copy link

Supported:

  • work with monitoring notifications channels
  • work with monitoring triggers
  • create system monitoring one time template link for compute

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/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

any chance to put it in keystone catalog?

Copy link
Author

Choose a reason for hiding this comment

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

done

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/",
Copy link
Collaborator

Choose a reason for hiding this comment

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

version number should not be a part of url settings

Copy link
Author

Choose a reason for hiding this comment

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

done

Type: schema.TypeString,
Required: true,
ForceNew: true,
Description: "Type of channel: email or sms.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add ValidateFunc to check that only "email" or "sms" is specified?

}
d.SetId(ch.Channel.ID)

return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should return resourceChannelRead(ctx, d, meta) in create and update methods

return diag.Errorf("Error update VKCS monitoring channel: %s", err)
}

return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should set computed (and other fields except id) in resourceTemplaterRead method

return &schema.Resource{
CreateContext: resourceTemplaterCreate,
ReadContext: resourceTemplaterRead,
UpdateContext: resourceTemplaterUpdate,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error deleting


err = triggerDelete(MonitoringV1Client, config.GetTenantID(), d.Id()).extractErr()
if err != nil {
return diag.Errorf("Error de VKCS monitoring trigger: %s", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error deleting

}
_, err = channelUpdate(MonitoringV1Client, config.GetTenantID(), d.Id(), &chn).extract()
if err != nil {
return diag.Errorf("Error update VKCS monitoring channel: %s", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error updating

}
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error retrieving

},
},
})
}
Copy link
Collaborator

@paaanic paaanic Mar 30, 2023

Choose a reason for hiding this comment

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

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]
}
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty line

},
},
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as for channel test


func createChannelURL(c ContainerClient, pid string) string {
return c.ServiceURL(pid, "notification_channels")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Custom urls are usually defined in urls.go


func (r deleteChannelResult) extractErr() error {
return r.Err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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
Copy link
Collaborator

Choose a reason for hiding this comment

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

channelGet

return
}

// triggerDelete performs request to delete trigger
Copy link
Collaborator

Choose a reason for hiding this comment

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

channelDelete

r.Header = result.Header
}
return r
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't stick to common way with returning (r commonTemplateResult)?

"channel_type": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are able to update channel_type/address then we should remove ForceNew

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