-
Notifications
You must be signed in to change notification settings - Fork 389
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
Don't ignore options diff in synthetics resource #707
Conversation
We ignore the diff completely when options_list is set, which sounds like unwanted behavior. Closes #697
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
@therve can we add a regression test case?
It's going to be tough because we ignore diffs on those particular fields in the tests. I'll have a look. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -263,6 +263,23 @@ func TestAccDatadogSyntheticsTCPTest_Basic(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccDatadogSyntheticsTCPLegacyOptionsTest_Basic(t *testing.T) { |
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.
Does this test give us anything specific?
The other two look to cover the case of
- ensuring we can still set the old options
- ensuring that if you upgrade from old -> new, the old is no longer present
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.
Good point, updated a test instead. We do cover the 2 point.
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.
LGTM, left one question about a test
We ignore the diff completely when options_list is set, which sounds
like unwanted behavior.
Closes #697