-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/opsgenie: Descriptions for Teams #11391
provider/opsgenie: Descriptions for Teams #11391
Conversation
tombuildsstuff
commented
Jan 24, 2017
- Updating the OpsGenie SDK to the latest and greatest
- Adding the Description field to a Team
- Acceptance & Import Tests
- Documentation
Tests pass
|
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 like to see a test that doesn't have a description as I am curious as to how this acts.
Apart from that, looks sane :)
Members: expandOpsGenieTeamMembers(d), | ||
Name: name, | ||
Description: description, | ||
Members: expandOpsGenieTeamMembers(d), |
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.
If we pass an empty string here is this ok?
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.
yep - it's caught by the HCL (which picks it up as a null value) - there's also tests for omitting/empty/completed values
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.
yep - it's caught by the HCL (which picks it up as a null value) - there's also tests for omitting/empty/completed values
Members: expandOpsGenieTeamMembers(d), | ||
Id: d.Id(), | ||
Name: name, | ||
Description: description, |
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.
If we pass an empty string, is this ok?
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.
it's caught by the HCL (which picks it up as a null value)
@stack72 There's a test To clarify - when I go from the following state:
to the following state:
The plan is empty - presumably because the HCL parser ignores empty values. Is that sufficient, or is it worth adding some validation to the description field? In addition the field is marked as Tests pass:
|
In that case - all LGTM :) Thanks @tombuildsstuff P. |
* updating the opsgenie dependency * Adding description to an OpsGenie team * Description for Teams * Added tests for an empty description
* updating the opsgenie dependency * Adding description to an OpsGenie team * Description for Teams * Added tests for an empty description
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |