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.

Sign up for GitHub

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

Jump to bottom

add grafana_oncall_user_notification_rule resource #1653

Merged
merged 24 commits into from
Jul 8, 2024
Merged

add grafana_oncall_user_notification_rule resource #1653

merged 24 commits into from
Jul 8, 2024

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Jun 19, 2024 •
edited
Loading

Related to the Terraform portion of grafana/oncall#4410

Closes grafana/oncall#1193

Once a new provider version is released, I will open a PR in https://github.com/grafana/crossplane-provider-grafana to generate the related Crossplane resources

Tested on my local stack all of the following cases and all work as expected:

  • creating several grafana_oncall_user_notification_rule, various types and positions
  • updates
  • deletions
  • importing
  • force recreates - changing either the user_id or important value for a resource, should delete the old resource and create a new one

@github-actions GitHub Actions
Copy link

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

@joeyorlando joeyorlando mentioned this pull request Jun 19, 2024
Merged
@julienduchesne
Copy link
Member

julienduchesne commented Jun 19, 2024 •
edited
Loading

This is going to be an annoying comment but we're trying to get rid of the old TF framework (which is what this PR is using). New resources look like this: https://github.com/grafana/terraform-provider-grafana/blob/main/internal/resources/cloud/resource_cloud_org_member.go

@joeyorlando
Copy link
Contributor Author

thanks for pointing that out @julienduchesne, will take a look 👍

@joeyorlando joeyorlando mentioned this pull request Jul 2, 2024
Merged
3 tasks
@joeyorlando joeyorlando marked this pull request as ready for review July 5, 2024 16:34
@joeyorlando joeyorlando requested review from a team as code owners July 5, 2024 16:34
@joeyorlando joeyorlando mentioned this pull request Jul 5, 2024
Closed
joeyorlando added a commit to grafana/oncall that referenced this pull request Jul 5, 2024
@joeyorlando
don't force create default user notification policies (#4608)
abedea7 
# What this PR does

Related to #4410

The changes in this PR are a prerequisite to
grafana/terraform-provider-grafana#1653. See the
conversation
[here](https://raintank-corp.slack.com/archives/C04JCU51NF8/p1719806995902499?thread_ts=1719520920.744319&cid=C04JCU51NF8)
for more context on why we decided to move away from always creating
default personal notification rules for users.

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] Added the relevant release notes label (see labels prefixed w/
`release:`). These labels dictate how your PR will
    show up in the autogenerated release notes.
julienduchesne
julienduchesne reviewed Jul 7, 2024
View reviewed changes
Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

Other than the failing linter + an import test, this looks great! Thanks for redoing everything in the new plugin framework. Much appreciated.

internal/resources/oncall/resource_user_notification_rule_test.go Outdated Show resolved Hide resolved
vadimkerr
vadimkerr reviewed Jul 8, 2024
View reviewed changes
internal/resources/oncall/resource_user_notification_rule.go
var (
resourceUserNotificationRuleName = "grafana_oncall_user_notification_rule"

userNotificationRuleTypeOptions = []string{
Copy link
Member

Choose a reason for hiding this comment

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

should MS Teams be in this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in bdc35ec (I'll also update our public API docs to make a mention that this is available as well (only in Grafana Cloud))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/grafana/oncall/pull/4628/files#diff-7225c8889a4ef0527f6d724edcb8482d7f3f89e41e8e6e68e393214d33f8aae1R38

internal/resources/oncall/resource_user_notification_rule.go Outdated Show resolved Hide resolved
@joeyorlando joeyorlando mentioned this pull request Jul 8, 2024
Merged
3 tasks
julienduchesne
julienduchesne approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

👍

@julienduchesne julienduchesne merged commit 3b74b81 into main Jul 8, 2024
27 checks passed
@julienduchesne julienduchesne deleted the jorlando/add-oncall-user-notification-policy-resource branch July 8, 2024 16:35
@julienduchesne julienduchesne mentioned this pull request Jul 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienduchesne julienduchesne julienduchesne approved these changes

@vadimkerr vadimkerr Awaiting requested review from vadimkerr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow configuring users through Terraform
3 participants
@joeyorlando @julienduchesne @vadimkerr