fix: alert rule and tests association #168
Merged
+183
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
te-hercules-ci
bot
requested review from
jabrowne-te,
joaomper-TE,
mpragosa-te,
pedro-te,
phpinhei-te,
sfreitas-te,
shahid-te and
vascmart-te
brumarqu-te
changed the title
jabrowne-te
reviewed
May 10, 2024
Co-authored-by: Jack Browne <[email protected]>
brumarqu-te
force-pushed
the
fix_alert_rule_tests
branch
from
bb62dcf to
03f213c
Compare
pedro-te
approved these changes
May 10, 2024
joaomper-TE
reviewed
May 10, 2024
| @@ -0,0 +1,34 @@ | |||
| data "thousandeyes_agent" "amsterdam" { | |||
| agent_name = "Amsterdam, Netherlands" | |||
There was a problem hiding this comment.
nice, more ATs for this repo! 💪
| }, | ||
| checkUpdateFunc: []resource.TestCheckFunc{ | ||
| resource.TestCheckResourceAttr(alertRuleResourceName, "rounds_violating_required", "4"), | ||
| resource.TestCheckResourceAttr(alertRuleResourceName, "test_ids.#", "1"), |
There was a problem hiding this comment.
We may want to start adding some comments to these Acceptance tests - the ones that I created also do not have them and they should!
It's not immediate to understand that here we are making sure that if an alertRule is updated, them the tests associated with it are not cleared. Can you add a comment?
There was a problem hiding this comment.
tried to reflect that in the test name but i guess i failed miserably 😅
i've added the comments, take another look when possible
joaomper-TE
reviewed
May 10, 2024
brumarqu-te
force-pushed
the
fix_alert_rule_tests
branch
from
cf60211 to
08e1aab
Compare
joaomper-TE
approved these changes
May 10, 2024
jabrowne-te
approved these changes
May 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #158
This issue is happening because we currently allow the alert rule <-> test association to be established from both perspectives. From the alert rule update perspective, it will clear any test association if the list of testIds is not sent in the request.
This PR changes the alert rule
test_idsschema attribute to be computed so that it is not considered in the TF plan but it's still retrieved from the alert rule get by id operation and passed along the alert rule update request.Also, since the alert rule returns a field
testswhen getting by id and receives a fieldtestIdswhen updating the alert rule, had to implement a map from the GenericTest to get only the testId.One drawback of this approach (not a huge one IMO) is that the state may not always be accurate. This is because a test <-> alert rules association can be changed from the test perspective and not get reflected on the alert rule perspective. Anyways, it will eventually update the state when the next alert rule update is performed.