Remove some suppression of AZC0006 and AZC0007

[pshao25]

There are some suppressions in the repo but the targeting code is not violating the rules. We could remove these suppressions.
There was a PR before and for some reason it was put off. But these two are actually doing the same thing.

We might need to change the analyzer to loosen the rules according to the agreement here. So I want to make sure the language repo is clean.

[azure-sdk]

API change check

API changes are not detected in this pull request.

[jsquire]

@pshao25: Please help us understand the motivation for making this change. Were there analyzer changes that loosened rules? What is motivating you to go find these and make this change?

[jsquire]

Until we understand the motivation behind a change that crosses multiple products and see a clean live test run from all impacted libraries, this should not move forward.

[pshao25]

@pshao25: Please help us understand the motivation for making this change. Were there analyzer changes that loosened rules? What is motivating you to go find these and make this change?

Were there analyzer changes that loosened rules?

No.

But it might come from previous change. I just found that I had a PR before and for some reason it was put off.

What is motivating you to go find these and make this change?

I'm going to change the analyzer to loosen the rules as we agreed here. So I want to make sure the language repo is clean.

[jsquire]

I'm going to change the analyzer to loosen the rules as we agreed here. So I want to make sure the language repo is clean.

Going forward, please include this context in your pull requests. We should be able to look at the description and understand why this change is being made - particularly if associated with other work that will impact the entire repository.

[pshao25]

I'm going to change the analyzer to loosen the rules as we agreed here. So I want to make sure the language repo is clean.

Going forward, please include this context in your pull requests. We should be able to look at the description and understand why this change is being made - particularly if associated with other work that will impact the entire repository.

I updated the description. But no matter we will change the analyzer or not, we should remove these suppressions, I think. The targeting code is not violating any rules. If we keep these suppression, we cannot catch the real violation in the future.

[jsquire]

But no matter we will change the analyzer or not, we should remove these suppressions, I think.

Agreed. If we don't actually need these suppressions to pass CI, we should remove them. Before we move forward, I do want to see all of the CI and Live test pipelines for the libraries that were touched here running and passing.

[pshao25]

But no matter we will change the analyzer or not, we should remove these suppressions, I think.

Agreed. If we don't actually need these suppressions to pass CI, we should remove them. Before we move forward, I do want to see all of the CI and Live test pipelines for the libraries that were touched here running and passing.

The CIs are all green. Did you see any failure?

[jsquire]

But no matter we will change the analyzer or not, we should remove these suppressions, I think.

Agreed. If we don't actually need these suppressions to pass CI, we should remove them. Before we move forward, I do want to see all of the CI and Live test pipelines for the libraries that were touched here running and passing.

The CIs are all green. Did you see any failure?

The CIs look good. I do not see any Live pipelines run. Those also need to be validated.

[pshao25]

But no matter we will change the analyzer or not, we should remove these suppressions, I think.

Agreed. If we don't actually need these suppressions to pass CI, we should remove them. Before we move forward, I do want to see all of the CI and Live test pipelines for the libraries that were touched here running and passing.

The CIs are all green. Did you see any failure?

The CIs look good. I do not see any Live pipelines run. Those also need to be validated.

What live pipelines you are mentioning? If an RP has live tests, then these tests will be in the pipeline. Therefore, as long as we pass the ci, we pass all the tests including live tests.

If you have any other live tests, could you send me the link how to trigger? And I'm really curious if it is a must, why it is not a part of CI, how could customers know that when they change their SDK?

[jsquire]

Therefore, as long as we pass the ci, we pass all the tests including live tests.

That is very, very far from the truth. All of CI runs as part of the public set of pipelines. All live tests run for internals only because they have access to secrets. Community contributors intentionally cannot run them. It is the responsibility of our team doing reviews to determine if a change is sufficiently impactful that a live run is needed and validate the PR contents first to be sure that it is safe to do so.

While, in theory, the analyzers should only impact the build step and that should be consistent across pipelines, we should always validate the live tests for fundamental changes.
I'll kick off the Event Hubs and Service Bus legs below. The rest that I leave to you, the naming pattern is consistent.

[jsquire]

/azp run net - eventhub - tests

[jsquire]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pshao25]

/azp run net - agrifood - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pshao25]

/azp run net - anomalydetector - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pshao25]

/azp run net - devcenter - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pshao25]

/azp run net - search - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pshao25]

@jsquire Thanks for the information! I triggered the left ones. For the failed ones from my trigger, all have been failed for a long time. Some even never succeed.

It is the responsibility of our team doing reviews to determine if a change is sufficiently impactful that a live run is needed and validate the PR contents first to be sure that it is safe to do so.

I'm still curious about the criterion to determine if a change needs a live run, so that we could understand why removing suppressions cross the line for live tests. And how we could make sure that reviewers follow this? I'm asking because if they did follow this, these live tests won't fail for such a long time.

[jsquire]

@jsquire Thanks for the information! I triggered the left ones. For the failed ones from my trigger, all have been failed for a long time. Some even never succeed.

It is the responsibility of our team doing reviews to determine if a change is sufficiently impactful that a live run is needed and validate the PR contents first to be sure that it is safe to do so.

I'm still curious about the criterion to determine if a change needs a live run, so that we could understand why removing suppressions cross the line for live tests. And how we could make sure that reviewers follow this? I'm asking because if they did follow this, these live tests won't fail for such a long time.

Unless it's a trivial change that impacts just the surface area and is fully tested by mocks or recorded tests, run the live pipeline (after looking over the changes and assessing that it is safe to do so.) Not sure? Run the live pipeline.

[pshao25]

@jsquire Thanks for the information! I triggered the left ones. For the failed ones from my trigger, all have been failed for a long time. Some even never succeed.

It is the responsibility of our team doing reviews to determine if a change is sufficiently impactful that a live run is needed and validate the PR contents first to be sure that it is safe to do so.

I'm still curious about the criterion to determine if a change needs a live run, so that we could understand why removing suppressions cross the line for live tests. And how we could make sure that reviewers follow this? I'm asking because if they did follow this, these live tests won't fail for such a long time.

Unless it's a trivial change that impacts just the surface area and is fully tested by mocks or recorded tests, run the live pipeline (after looking over the changes and assessing that it is safe to do so.) Not sure? Run the live pipeline.

So you are saying it is a subjective judgment. Let me just ask this simple question, do you think I should fix the errors for these failures, though it is not caused from my change because:

1. The tests already run means it passed the compilation. (also the CI pass means it passed the compilation too)
2. As I stated above, all have been failed for a long time, some even never succeed, which means people just ignore them before, though I don't know why they ignore.
3. From the error message, it cannot read some env variables, it probably comes from the pipeline itself has flaw.

[jsquire]

So you are saying it is a subjective judgment.

Everything in life is a judgment call. There are very few absolutes. 😄

do you think I should fix the errors for these failures

Nope. If we're certain that you didn't introduce new failures, then there's no need to expand the scope to fix failures. If its something small and easy and you want to help out - go for it. But we don't need to force them to pass if they fail with the same errors they were already seeing.

From the error message, it cannot read some env variables, it probably comes from the pipeline itself has flaw.

Agreed. I think we've met our burden here.