fix: default alert rule not being read #151
Merged
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
shahid-te
previously approved these changes
Dec 6, 2023
raul-te
approved these changes
Dec 6, 2023
phpinhei-te
previously approved these changes
Dec 7, 2023
brumarqu-te
reviewed
Dec 7, 2023
brumarqu-te
previously approved these changes
Dec 7, 2023
7abcbc3
joaomper-TE
commented
Dec 7, 2023
| @@ -25,3 +25,13 @@ jobs: | |||
| run: | | |||
| git diff --compact-summary --exit-code || \ | |||
| (echo; echo "Unexpected difference in directories after code generation. Run 'go generate' command and commit."; exit 1) | |||
|
|
|||
| unit: | |||
There was a problem hiding this comment.
we had some UTs but we were not running them 😄
joaomper-TE
commented
Dec 7, 2023
| @@ -53,68 +53,6 @@ func expandAlertRules(alertRules *[]thousandeyes.AlertRule) *[]thousandeyes.Aler | |||
| return ret | |||
| } | |||
|
|
|||
| func expandBGPMonitors(v interface{}) thousandeyes.BGPMonitors { | |||
joaomper-TE
commented
Dec 7, 2023
| output, err = FixReadValues(testsInput, "tests") | ||
| if err != nil { | ||
| t.Errorf("tests input returned error: %s", err.Error()) | ||
| } | ||
| if output != nil { | ||
| if reflect.DeepEqual(output, testsTarget) != true { |
There was a problem hiding this comment.
Just fixing this, nothing changed as part of this PR. This was breaking for a long time already. For the tests we do nothing, so the output should be the same as the input
shahid-te
approved these changes
Dec 7, 2023
brumarqu-te
approved these changes
Dec 12, 2023
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.
This fixes #111 .
It seems to conflict with the fix that was done here for #73. However, was trying out the scenarios from #73 locally:
alerts_enabledset totrueand without specifying any alert rulealerts_enabledset tofalseand with 2 alert rules, one of them being a default alert ruleand both Terraform plan/apply, as well as subsequent terraform plans, ran ok (and the subsequent terraform plans showed no changes).
@raul-te not sure if you have any objection or know anything else that I should further test before merging this. Because it does seems that filtering out the Default Alert Rules is no longer needed...
NOTE: We do need some tests for this repo though, as we should have more visibility when fixing something do make sure that we don't introduce any regression.