-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add opsgenie destination resource #176
base: main
Are you sure you want to change the base?
Conversation
ForceNew: true, | ||
Description: "Basic auth used to authenticate with the Opsgenie instance", | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add descriptions for these fields too? it's on my mind since I just went through and did it for a bunch of things
Description: "Basic auth used to authenticate with the Opsgenie instance", | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"username": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the username wasn't used with OpsGenie destination?
@@ -24,7 +24,7 @@ func init() { | |||
|
|||
testProject = os.Getenv("LIGHTSTEP_PROJECT") | |||
if testProject == "" { | |||
testProject = "terraform-provider-tests" | |||
testProject = "terraform-provider-test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! I didn't realize this was already set automatically, I had just made an env variable
|
||
func resourceOpsgenieDestination() *schema.Resource { | ||
return &schema.Resource{ | ||
CreateContext: resourceOpsgenieDestinationCreate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to add a note for future readers about why there's no Update entry here?
return resourceDestinationRead(ctx, d, m) | ||
} | ||
|
||
func resourceOpsgenieDestinationImport(ctx context.Context, d *schema.ResourceData, m interface{}) ([]*schema.ResourceData, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooc, how do we test this sort of thing? are the acceptance tests sufficient or do you have to actually try it using the terraform import command on a live system?
testAccCheckWebhookDestinationExists("lightstep_opsgenie_destination.opsgenie", &destination), | ||
resource.TestCheckResourceAttr("lightstep_opsgenie_destination.opsgenie", "destination_name", "my-destination"), | ||
resource.TestCheckResourceAttr("lightstep_opsgenie_destination.opsgenie", "url", "https://example.com"), | ||
resource.TestCheckResourceAttr("lightstep_opsgenie_destination.opsgenie", "auth.0.username", ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if they do specify a username? is that an option we even want to expose if it will never be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I waffled on this. On one hand opsgenie is accepting a basic auth token so it makes sense to support username if they ever start using it. On the other hand we could hide this from the user in terraform just like we do in the UI. (It wouldn't be hidden in the public API though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to hide it in terraform, assuming that doesn't create more work for you, in the interest of making terraform as minimally confusing as possible.
destinationConfig := ` | ||
resource "lightstep_opsgenie_destination" "opsgenie" { | ||
project_name = ` + fmt.Sprintf("\"%s\"", testProject) + ` | ||
destination_name = "my-destination" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just to make this more realistic, maybe add some spaces in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments but nothing blocking, lgtm
Adds a resource to manage opsgenie destinations.