This repository has been archived by the owner on Dec 6, 2022. It is now read-only.
Move ADLS ingestion app details from local to global settings #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The GitHub validation check will fail until my changes in CEDAR/GitHub are accepted. |
|
I made API breaking changes in CEDAR/Core for CEDAR/GitHub, thus the validation for CEDAR.Core.Collector-GitHub.Validation failed. I added the old API in addition to the new API in this PR for the check to pass. I will remove the old API in a future PR. |
|
I have added support for both the old and new AdlsClientWrapper APIs. I have thoroughly tested that the old and new versions of ADO and CEDAR/GitHub work correctly, but I intend to deprecate the old AdlsClientWrapper API as soon as I can get my PRs through in the different repos. |
Merge with origin/main. Also add the new settings (AdlsAccount and AdlsTenantId) to the config. Finally, Make config more structured (a JObject).
kivancmuslu
approved these changes
Apr 12, 2021
kivancmuslu
pushed a commit
to microsoft/CEDAR.GitHub.Collector
that referenced
this pull request
Apr 12, 2021
These changes are required due to the changes in [this ](microsoft/CEDAR.Core.Collector#42) CEDAR/Core pull request. The local and global settings templates all needed to be updated to reflect the movement of the ADLS ingestion app details from local to global settings. This involved moving one key `AdlsIngestionApplicationId` out of local settings. The local and global settings barebones templates needed to be updated because they contained incorrect formatting (an issue I previously pushed to CEDAR/GitHub, my IDE was not configured correctly). In `ServiceStartup.Configure()`, I added the global settings string to builder services so that I could access it in `AdlsClientWrapper()`. Finally, I updated `GitHubConfigManagerTests()` to use the newly added ADLS ingestion app details in global settings and the updated `AdlsClientWrapper()` constructor.
AllainPL
pushed a commit
that referenced
this pull request
Sep 21, 2022
This PR moves the ADLS ingestion application details from local.settings.json to Settings.json. To do this, I pass the global settings JSON string to AdlsClientWrapper and parse it for two new keys `AdlsIngestionApplicationId` and `AdlsIngestionApplicationSecretEnvironmentVariable`. The former is moved directly from local settings, the latter contains the environment variable name of the ADLS secret. The global settings string is accessed from builder services (changes in `ServiceStartup.Configure()` in CEDAR/GitHub and ADO will be made to add the string to builder services). I also added a utility class `JTokenUtility` that allows me to perform checks on JTokens just like `string.IsNullOrEmpty()` and `string.IsNullOrWhiteSpace()`. This is great because it cleans up the checks in AdlsClientWrapper() (number of checks needed reduced from 4 -> 2) and will allow me to clean up other code in the collectors in future PRs.
AllainPL
pushed a commit
to microsoft/CEDAR.GitHub.Collector
that referenced
this pull request
Sep 21, 2022
These changes are required due to the changes in [this ](microsoft/CEDAR.Core.Collector#42) CEDAR/Core pull request. The local and global settings templates all needed to be updated to reflect the movement of the ADLS ingestion app details from local to global settings. This involved moving one key `AdlsIngestionApplicationId` out of local settings. The local and global settings barebones templates needed to be updated because they contained incorrect formatting (an issue I previously pushed to CEDAR/GitHub, my IDE was not configured correctly). In `ServiceStartup.Configure()`, I added the global settings string to builder services so that I could access it in `AdlsClientWrapper()`. Finally, I updated `GitHubConfigManagerTests()` to use the newly added ADLS ingestion app details in global settings and the updated `AdlsClientWrapper()` constructor.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the ADLS ingestion application details from local.settings.json to Settings.json.
To do this, I pass the global settings JSON string to AdlsClientWrapper and parse it for two new keys
AdlsIngestionApplicationIdandAdlsIngestionApplicationSecretEnvironmentVariable. The former is moved directly from local settings, the latter contains the environment variable name of the ADLS secret. The global settings string is accessed from builder services (changes inServiceStartup.Configure()in CEDAR/GitHub and ADO will be made to add the string to builder services).I also added a utility class
JTokenUtilitythat allows me to perform checks on JTokens just likestring.IsNullOrEmpty()andstring.IsNullOrWhiteSpace(). This is great because it cleans up the checks in AdlsClientWrapper() (number of checks needed reduced from 4 -> 2) and will allow me to clean up other code in the collectors in future PRs.