Address non-Terra concerns for Terra mitigations #4
Conversation
| @@ -334,17 +334,53 @@ public string GetNodeManagedIdentityResourceId(TesTask task, string globalManage | |||
| task.Resources?.GetBackendParameterValue(TesResources.SupportedBackendParameters | |||
| .workflow_execution_identity); | |||
|
|
|||
| if (string.IsNullOrEmpty(workflowId)) | |||
There was a problem hiding this comment.
Is this version of this function used anywhere?
There was a problem hiding this comment.
Currently it is in 46 places in this repo https://github.com/search?q=repo%3Amicrosoft%2Fga4gh-tes%20string.IsNullOrEmpty&type=code
Generally, especially with external inputs where whitespace is truly meaningless or will lead to errors, we use IsNullOrWhiteSpace, but it's not been a hard-and-fast rule and there's been no effort to cleanup/standardize the usages of each.
There was a problem hiding this comment.
Oh, sorry for not being clear, I meant GetNodeManagedIdentityResourceId(TesTask task, string globalManagedIdentity). I see that we now have both that and GetNodeManagedIdentityResourceId(string globalManagedIdentity, TesTask task) and am wondering if we need both. If we do, I suggest that they be named differently to reflect their different behavior (preferring the global managed identity vs the workflow identity).
There was a problem hiding this comment.
Sorry I misinterpreted the context based on the selected line.
The functionality is the same, and in both cases the first parameter is favored over the second. My Achillies heel has always been naming (yes, it may be stereotypical, but it definitely applies to me). I don't have a problem with both methods being overrides of each other by sharing a name, and I wasn't confused but I have to recognize that it can be confusing. I skipped my initial impulse to add a boolean parameter to indicate the preference, since that would have required producing a name 😉
There was a problem hiding this comment.
As to your original question (now in the correct context) yes, each of the two methods are each used once (and additionally each are tested). The global-first method is now used when creating the environment variable (for runner bootstrapping, that ordering is to ensure access to tes-internals non-task-specific storage areas outside of Terra deployments). The task-first method continues to be used when creating the runner task json (since that covers task-specific operations by definition).
No description provided.