Skip to content
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

Bump monitor API version #4507

Merged

Conversation

AndreiBarbuOz
Copy link
Contributor

@AndreiBarbuOz AndreiBarbuOz commented Dec 20, 2024

What this PR does

Update the version of the Microsoft.Insights/scheduledQueryRules to catch up with https://learn.microsoft.com/en-us/azure/templates/microsoft.insights/change-log/scheduledqueryrules#2024-01-01-preview

The new version comes with some features, including the addition of a .spec.identity field which allows using an existing user assignment identity when the service executes the query. This was captured in a focused test, which creates the necessary prerequisites before instantiating the object.

How does this PR make you feel?

gif

Checklist

  • this PR contains tests
  • this PR contains YAML Samples

@AndreiBarbuOz AndreiBarbuOz marked this pull request as draft December 20, 2024 16:48
@AndreiBarbuOz AndreiBarbuOz force-pushed the chore/bump-monitor-api-version branch from d13237b to dc357be Compare January 4, 2025 19:37
@AndreiBarbuOz AndreiBarbuOz marked this pull request as ready for review January 4, 2025 21:28
@AndreiBarbuOz
Copy link
Contributor Author

@microsoft-github-policy-service agree company="UiPath"

Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few minor comments below.

.golangci.yml Outdated Show resolved Hide resolved
@AndreiBarbuOz AndreiBarbuOz force-pushed the chore/bump-monitor-api-version branch from 771df97 to 7884677 Compare January 7, 2025 04:57
Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a minor thing without which CI won't pass is that you're missing a recording for sample test. You can run TEST_FILTER=Test_Samples_CreationAndDeletion task controller:test-integration-envtest command in root which'll test your latest sample(s) and create a recording.

@matthchr
Copy link
Member

Checking if you'd like this merged (or need help?) let us know!

@theunrepentantgeek
Copy link
Member

We're going to publish the v2.13 release of ASO very soon and it would be good to include these in that release.

Do you think you can sort out the CI issues in the next couple of days (say, by Thursday this week)?

@theunrepentantgeek
Copy link
Member

Did you miss committing the recordings from the tests that check the samples? CI is failing with these errors:

[produce-markdown-summary] - Test failed: Test_Samples_CreationAndDeletion/Test_Insights_v1api20220615_CreationAndDeletion
[produce-markdown-summary] === TEST OUTPUT ===
[produce-markdown-summary] === RUN   Test_Samples_CreationAndDeletion/Test_Insights_v1api20220615_CreationAndDeletion
[produce-markdown-summary] === PAUSE Test_Samples_CreationAndDeletion/Test_Insights_v1api20220615_CreationAndDeletion
[produce-markdown-summary] === CONT  Test_Samples_CreationAndDeletion/Test_Insights_v1api20220615_CreationAndDeletion
[produce-markdown-summary]     kube_per_test_context.go:167: creating recorder: required environment variable "AZURE_SUBSCRIPTION_ID" was not supplied
[produce-markdown-summary] --- FAIL: Test_Samples_CreationAndDeletion/Test_Insights_v1api20220615_CreationAndDeletion (0.00s)
[produce-markdown-summary] === END TEST OUTPUT ===
[produce-markdown-summary] - Test failed: Test_Samples_CreationAndDeletion/Test_Insights_v1api20240101preview_CreationAndDeletion
[produce-markdown-summary] === TEST OUTPUT ===
[produce-markdown-summary] === RUN   Test_Samples_CreationAndDeletion/Test_Insights_v1api20240101preview_CreationAndDeletion
[produce-markdown-summary] === PAUSE Test_Samples_CreationAndDeletion/Test_Insights_v1api20240101preview_CreationAndDeletion
[produce-markdown-summary] === CONT  Test_Samples_CreationAndDeletion/Test_Insights_v1api20240101preview_CreationAndDeletion
[produce-markdown-summary]     kube_per_test_context.go:167: creating recorder: required environment variable "AZURE_SUBSCRIPTION_ID" was not supplied
[produce-markdown-summary] --- FAIL: Test_Samples_CreationAndDeletion/Test_Insights_v1api20240101preview_CreationAndDeletion (0.00s)
[produce-markdown-summary] === END TEST OUTPUT ===

This usually means missing recordings for the tests.

If that's not it, let me know and I'll investigate further by checking out your PR locally.

@petrepopescu21
Copy link
Contributor

petrepopescu21 commented Feb 4, 2025

@theunrepentantgeek The tests were actually failing because of a non-retryable 400 error received when the role assignment was not ready (the samples use a managed identity).

Had to add a case to the ErrorClassifier for ScheduledQueryRuns (found your PR here) and I've since committed the test recording as well.

Please let me know if the classifier change needs to be a separate PR or if they can be merged together.

@matthchr
Copy link
Member

matthchr commented Feb 4, 2025

Please let me know if the classifier change needs to be a separate PR or if they can be merged together.

Together should be fine.

@matthchr
Copy link
Member

matthchr commented Feb 4, 2025

/ok-to-test sha=0125a98

@petrepopescu21
Copy link
Contributor

There was a sample moved which I missed, caused a "missing recording" event.
Also, the CRUD recording had some tags on the RG. I've re-recorded the test and it should now be correct.

Sorry @matthchr for the back and forth on this

@theunrepentantgeek
Copy link
Member

I think we're getting close to a clean build. Getting a single lint error - maybe this indicates a missing assertion?

[controller:lint] internal/controllers/crd_insights_scheduledqueryrule_20240101preview_test.go:74:2: ineffectual assignment to armId (ineffassign)
[controller:lint] 	armId := *roleAssignment.Status.Id
[controller:lint] 	^

@matthchr
Copy link
Member

matthchr commented Feb 5, 2025

/ok-to-test sha=7e5ed65

@matthchr
Copy link
Member

matthchr commented Feb 5, 2025

/ok-to-test sha=62123ec

@theunrepentantgeek theunrepentantgeek added this pull request to the merge queue Feb 5, 2025
Merged via the queue into Azure:main with commit fd62017 Feb 6, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Recently Completed
Development

Successfully merging this pull request may close these issues.

5 participants