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.

Sign up for GitHub

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

Jump to bottom

TestCase: Fix TC for Log Analytics #18205

Closed
wants to merge 8 commits into from
Closed

TestCase: Fix TC for Log Analytics #18205

wants to merge 8 commits into from

Conversation

neil-yechenwei
Copy link
Contributor

@neil-yechenwei neil-yechenwei commented Sep 1, 2022 •
edited
Loading

This PR is to fix TCs for Log Analytics.

--- PASS: TestAccLogAnalyticsDataSourceWindowsEvent_complete (265.56s)
--- PASS: TestAccLogAnalyticsDataSourceWindowsEvent_basic (298.34s)
--- PASS: TestAccLogAnalyticsDataSourceWindowsEvent_requiresImport (307.01s)
--- PASS: TestAccLogAnalyticsDataSourceWindowsEvent_update (540.47s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_complete (304.24s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_basic (309.85s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_ingestion (312.96s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_requiresImport (357.57s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_update (558.49s)

@katbyte
Copy link
Collaborator

katbyte commented Sep 2, 2022

ie, why are we ignoring case now?

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Sep 2, 2022 •
edited
Loading

@katbyte , I thought of another way to fix these TCs. Please take another look. Thanks.

@katbyte
Copy link
Collaborator

katbyte commented Sep 2, 2022

if we are doing anything we should be correcting the case on read to the correct case expected not ignoring case

@neil-yechenwei
Copy link
Contributor Author

@katbyte , thanks for your suggestion. I've updated PR. Please take another look. Thanks.

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Sep 7, 2022 •
edited
Loading

@katbyte , thanks for your comment. I've updated PR. Please take another look. Thanks.

image

katbyte
katbyte requested changes Sep 9, 2022
View reviewed changes
internal/services/loganalytics/log_analytics_linked_storage_account_resource.go Outdated
Comment on lines 51 to 52
}, true),
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we flag these with 4.0 - we want all 4.0 changes flagged so 4.0 is just a matter of flipping the flag

Suggested change
}, true),
DiffSuppressFunc: suppress.CaseDifference,
}, !features.FourPointOhBeta),
DiffSuppressFunc: !features.FourPointOhBeta,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot use "DiffSuppressFunc: !features.FourPointOhBeta" since DiffSuppressFunc requires dynamic value. So I flag it via another way. Please take another look. Thanks.

@neil-yechenwei
Copy link
Contributor Author

@katbyte , thanks for your comment. I've updated code. Please take another look. Thanks.
image

katbyte
katbyte reviewed Sep 15, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

is this a duplicate of https://github.com/hashicorp/terraform-provider-azurerm/pull/18116/files ?

@neil-yechenwei
Copy link
Contributor Author

@katbyte , yes, but seems PR 18116 only fixed one issue. I fixed two issues.

One fix is setting ignoreCase of ValidateFunc to true to allow lower case in tf config to avoid breaking change.

Another fix is adding DiffSuppress to ignore the difference between the value in state file (ex: "CustomLogs") and the value in tf config (ex: "customlogs").

@katbyte
Copy link
Collaborator

katbyte commented Sep 28, 2022

Closing as i think #18116 has fixed the same issue, the code changes here seem to be the same. do let me know if i am mistaken

@katbyte katbyte closed this Sep 28, 2022
@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Sep 28, 2022 •
edited
Loading

@katbyte , this PR also fixed the TCs of azurerm_log_analytics_datasource_windows_event. The PR 18116 didn't fix the, Currently the TCs of azurerm_log_analytics_datasource_windows_event still failed. So could you re-open this PR and I will remove the part of azurerm_log_analytics_linked_storage_account? Or I have to open a new PR for it?

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Sep 28, 2022 •
edited
Loading

@katbyte , I filed a new PR to fix the TCs of azurerm_log_analytics_datasource_windows_event if you don't mind.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@katbyte katbyte Awaiting requested review from katbyte

Assignees
No one assigned
Labels
service/log-analytics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@neil-yechenwei @katbyte