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

Improve/insights samples #2827

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Conversation

mehighlow
Copy link
Contributor

Refers #2821 and #2822

What this PR does / why we need it:
This PR improves insights sample

Special notes for your reviewer:
None

@matthchr
Copy link
Member

matthchr commented Mar 30, 2023

We also have tests for each sample (to ensure that the samples are correct). The tests are done using HTTP recordings and the results are checked in.

These samples have two tests associated with them:
https://github.com/Azure/azure-service-operator/blob/main/v2/internal/controllers/recordings/Test_Samples_CreationAndDeletion/Test_Insights_v1api_CreationAndDeletion.yaml
and
https://github.com/Azure/azure-service-operator/blob/main/v2/internal/controllers/recordings/Test_Samples_CreationAndDeletion/Test_Insights_v1beta_CreationAndDeletion.yaml

In order to re-record those tests, you need to follow the instructions here. Mainly you need to set up the AZURE_SUBSCRIPTION_ID, AZURE_TENANT_ID, etc.

You don't need to run every integration test, if you just run the one sample test: Test_Samples_CreationAndDeletion, that will re-record the those files. The actual workflow to do this is:

  1. Set AZURE_SUBSCRIPTION_ID, etc vars
  2. Delete the two existing recording files (linked above).
  3. Run TEST_FILTER=Test_Samples_CreationAndDeletion task controller:test-integration-envtest

This should produce 2 new files (named the same as before but with different content) containing the HTTP recordings of successfully deploying that component + workspace.

It's a bit annoying but it ensures that our samples always work (because they're all tested). If setting that up is super odious let us know and we can come up with something to help you possibly.

@mehighlow
Copy link
Contributor Author

@matthchr, tests associated with samples have been added. PTAL.

@theunrepentantgeek
Copy link
Member

/ok-to-test: shar=3a44fea

@codecov-commenter
Copy link

Codecov Report

Merging #2827 (3a44fea) into main (c6e2cf5) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main    #2827    +/-   ##
========================================
  Coverage   52.59%   52.59%            
========================================
  Files        1222     1228     +6     
  Lines      561814   562022   +208     
========================================
+ Hits       295461   295577   +116     
- Misses     217981   218077    +96     
+ Partials    48372    48368     -4     

see 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

/ok-to-test sha=3a44fea

@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

Thanks for this contribution @mehighlow!

@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

/ok-to-test sha=3a44fea

1 similar comment
@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

/ok-to-test sha=3a44fea

@matthchr
Copy link
Member

matthchr commented Apr 3, 2023

/ok-to-test sha=507869a

@matthchr matthchr merged commit 13ba121 into Azure:main Apr 3, 2023
8 checks passed
emilychu9318 pushed a commit to emilychu9318/azure-service-operator that referenced this pull request Apr 11, 2023
* adds workspace object insights component refers to
* adds workspaceResourceReference as workspace-based appinsights requires it as the classic version of appinsights will be discontinued in Feb 2024
* adds workspaceResourceReference as workspace-based appinsights requires it as the classic version of appinsights will be discontinued in Feb 2024
* adds insights tests

---------

Co-authored-by: Mykhailo Zahlada <myzahlad@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants