Skip to content

Conversation

@ahsonkhan
Copy link
Contributor

Similar to #4727

Service SDKs should not be depending on each other. Likely just a copy/paste error from KeyVault tests which also had this issue.

@ahsonkhan ahsonkhan added Event Hubs test-reliability Issue that causes tests to be unreliable labels Jun 22, 2023
@ahsonkhan ahsonkhan self-assigned this Jun 22, 2023
@LarryOsterman
Copy link
Member

Are you also going to make the same change into the feature/amqp branch and in George's private branch?

I'm worried that this change is going to be lost if it's not present in all of the relevant places.

And why does this need to be made in main, rather than the amqp feature branch?

@ahsonkhan
Copy link
Contributor Author

And why does this need to be made in main, rather than the amqp feature branch?

Because the issue exists in main already (and I would like components in main to be buildable if I exclude attestation). Is there a different workflow here that I should follow, where the fix first needs to be made in a feature branch and then moved over?

@ahsonkhan ahsonkhan added test-enhancement and removed test-reliability Issue that causes tests to be unreliable labels Jun 22, 2023
@LarryOsterman
Copy link
Member

And why does this need to be made in main, rather than the amqp feature branch?

Because the issue exists in main already (and I would like components in main to be buildable if I exclude attestation). Is there a different workflow here that I should follow, where the fix first needs to be made in a feature branch and then moved over?

The eventhubs service is being developed in the feature/amqp branch, and by making this change in main, it runs a high risk of being overwritten when the feature/amqp branch comes in. It is safer to make the change everywhere the feature is being developed to avoid the potential of merge conflicts. If eventhubs was being built out of main, this wouldn't be a problem, but it is what it is.

Next stupid question: Why would you want to build everything in the azure-sdk-for-cpp repo without also building the attestation service? Also, it's not unreasonable for live service tests to require interacting between different client SDKs - we already see that with samples and the identity SDK.

In addition, the MHSM service has a strong dependency on the output of the attestation service, so having MHSM live tests depending on the attestation SDK is not at all unreasonable.

@ahsonkhan
Copy link
Contributor Author

Why would you want to build everything in the azure-sdk-for-cpp repo without also building the attestation service?

It isn't just about attestation, I wanted to build keyvault keys and run a test. I wanted the smallest build set from the repo. Similarly, if I want to build just attestation, it would be surprising to me that I would need to include the eventhubs or storage folders in the build, especially since these SDKs were built independently (without reliance on other SDKs being present).

To me, having tight integration between the SDKs is a sign of introduced complexity and tight coupling.

Also, it's not unreasonable for live service tests to require interacting between different client SDKs - we already see that with samples and the identity SDK.

I don't think that's a scenario we support in other language, or should support here. Unlike samples, live tests are meant to test the service client and that alone, they aren't integration tests for applications built using multiple SDKs together. The SDKs should be buildable and testable with only a dependency on Core, even if the samples have an exception and need identity (or higher-level samples that highlight multiple SDK clients working together).

In this case, it was purely incidental (copy/paste error), and not enabling some scenario we need.

In addition, the MHSM service has a strong dependency on the output of the attestation service, so having MHSM live tests depending on the attestation SDK is not at all unreasonable.

Maybe, but that would be surprising. We don't have that today, there's no need for linking such a dependency.

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Jun 22, 2023

The eventhubs service is being developed in the feature/amqp branch, and by making this change in main, it runs a high risk of being overwritten when the feature/amqp branch comes in. It is safer to make the change everywhere the feature is being developed to avoid the potential of merge conflicts.

Sure, I can make this change in the feature branch. See #4732

If eventhubs was being built out of main, this wouldn't be a problem, but it is what it is.

I thought we merged everything into main and the plan was to build out of that. Why do we have the eventhubs folder in main then if we are still using the feature branch? Should we remove it from main?

@LarryOsterman
Copy link
Member

The eventhubs service is being developed in the feature/amqp branch, and by making this change in main, it runs a high risk of being overwritten when the feature/amqp branch comes in. It is safer to make the change everywhere the feature is being developed to avoid the potential of merge conflicts.

Sure, I can make this change in the feature branch. See #4732

If eventhubs was being built out of main, this wouldn't be a problem, but it is what it is.

I thought we merged everything into main and the plan was to build out of that. Why do we have the eventhubs folder in main then if we are still using the feature branch? Should we remove it from main?

I believe George started working in feature/amqp and has continued to work there. It is what it is, the reality is that feature/amqp is still alive and kicking. This will hopefully only be for a few weeks more.

@LarryOsterman
Copy link
Member

Why would you want to build everything in the azure-sdk-for-cpp repo without also building the attestation service?

It isn't just about attestation, I wanted to build keyvault keys and run a test. I wanted the smallest build set from the repo. Similarly, if I want to build just attestation, it would be surprising to me that I would need to include the eventhubs or storage folders in the build, especially since these SDKs were built independently (without reliance on other SDKs being present).

To me, having tight integration between the SDKs is a sign of introduced complexity and tight coupling.

Also, it's not unreasonable for live service tests to require interacting between different client SDKs - we already see that with samples and the identity SDK.

I don't think that's a scenario we support in other language, or should support here. Unlike samples, live tests are meant to test the service client and that alone, they aren't integration tests for applications built using multiple SDKs together. The SDKs should be buildable and testable with only a dependency on Core, even if the samples have an exception and need identity (or higher-level samples that highlight multiple SDK clients working together).

In this case, it was purely incidental (copy/paste error), and not enabling some scenario we need.

In addition, the MHSM service has a strong dependency on the output of the attestation service, so having MHSM live tests depending on the attestation SDK is not at all unreasonable.

Maybe, but that would be surprising. We don't have that today, there's no need for linking such a dependency.

Today, we cannot build only one service, we only support building the entire tree. It stinks and I REALLY want to fix that during gallium.

And while it is a surprising result that one service would have a dependency on the output of another service, it's a reality in an interconnected world. There's really no difference between attestation and identity in this regard (attestation is an authorization service for MHSM and confidential ledger).

@antkmsft
Copy link
Member

I did not read the full discussion - I'll read it later, but why are you still developing in feature/amqp branch - didn't we merge the stuff to main? (and we did that mostly to eliminate the need for a feature branch - while the code is in beta, we can cut releases from main without problems).
If that is because of that issue that I've heard of - "CI is not finding the amqp package" - then I just checked and eventhubs still have its dependencies incorrectly written - let's fix them first until we can decisively say that our build and consumption model does not work - i.e. I even left the detailed feedback in #4571 on what should be fixed.

@ahsonkhan
Copy link
Contributor Author

Closing in favor of #4732

@ahsonkhan ahsonkhan closed this Jun 26, 2023
@ahsonkhan ahsonkhan deleted the ahsonkhan-patch-2 branch June 26, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants