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

[Azure.Core] Test Environment Exception When Optional Environment Variable Not Set #14869

Closed
jsquire opened this issue Sep 3, 2020 · 3 comments
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@jsquire
Copy link
Member

jsquire commented Sep 3, 2020

Observed Behavior

The TestEnvironment::GetRecordedOptionalVariable method reads the value of the environment variable, which may be null when not defined. That value is then passed into the RecordedVariableOptions when specified which attempts to parse the value using the ConnectionString type, causing an exception when validated due to the connection string value itself being null.

This was observed in the Service Bus library, when it's ServiceBusTestEnvironment::OverrideServiceBusConnectionString is not set.

Expected Behavior

  • Optional environment variables do not need to be set and do not result in an exception when null.
  • Using GetRecordedOptionalVariable with an optional environment variable is safe.

Implementation Thoughts

It looks as if the most appropriate place to adjust behavior would either be within the TestEnvironment, where null or empty values are not passed to the RecordedVariableOptions or within the options, adding a null or empty check before processing the connection string. I'm not sure of the intent for having overrides or extensions to RecordedVariableOptions where it may be desirable to invoke regardless of value.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 3, 2020
@jsquire jsquire added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Sep 3, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 3, 2020
@jsquire jsquire added this to the Backlog milestone Sep 3, 2020
@jsquire
Copy link
Member Author

jsquire commented Sep 3, 2020

@pakrym: If you can help me understand a bit more about the intention of RecordedVariableOptions, I'm happy to make the fix.

@pakrym
Copy link
Contributor

pakrym commented Sep 3, 2020

The intent of RecordedVariableOptions is for consumers to be able to describe what secrets are inside the value of the variable so we can sanitize them before storing it.

In this case skipping the entire code-path when no variable exists seems the right thing to do.

@jsquire
Copy link
Member Author

jsquire commented Sep 4, 2020

Fixed By #14906

@jsquire jsquire closed this as completed Sep 4, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-net that referenced this issue Jun 21, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

2 participants