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

OAK-9680 Add support for Azure SAS URIs to oak-segment-azure #486

Merged
merged 1 commit into from Feb 10, 2022

Conversation

jelmini
Copy link
Contributor

@jelmini jelmini commented Feb 8, 2022

Add support for SAS URIs to utility code.

Add support for SAS URIs to utility code.
@smiroslav
Copy link
Contributor

Looks good to me.

As a minor comment, not sure that introducing Environment class brings obvious benefits right now.

@jelmini
Copy link
Contributor Author

jelmini commented Feb 9, 2022

@smiroslav I had to introduce the Environment class to be able to test the code. Mockito cannot mock System.getenv() because it's a system class. Alternatively, it's possible to hack a workaround using reflection to obtain the underlying mutable Map from System.getenv(). This works with Java up until 15 with a big warning, but stops working from Java 16 forward (because "illegal reflective access" really becomes illegal).
Is there another approach for testing this code you could suggest?

@smiroslav
Copy link
Contributor

Thanks @jelmini for clarification. There are some test libraries that can inject values for system properties, but I like your approach better.

@smiroslav smiroslav merged commit f47c431 into apache:trunk Feb 10, 2022
@jelmini jelmini deleted the feature/sas_uri_support_2 branch February 14, 2022 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants