-
Notifications
You must be signed in to change notification settings - Fork 27
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
WX-1594 Support for private ACR access in Terra #715
Conversation
return null; | ||
} | ||
|
||
// TODO test with invalid guid for billing profile id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding error handling here in case either the Sam request or guid processing fail.
@jgainerdewar I don't know what is or isn't visible to you with regards to the checks, but the format check produced the following diagnostics:
So far I've only perused this PR, but I'm asking a couple of clarifications: The SAM API is called to obtain an azure managed identity that has |
@BMurri Luckily I can see the format check output, but I think you'll need to trigger it to run again with the latest changes, which should have fixed most of the issues (but may have created new ones!).
Yes, correct.
Yes, when it exists at all it will always be different. The |
@jgainerdewar I sent you an email |
/azr run |
@BMurri Still doing some final testing and planning to add more unit testing, but this is ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Only one point/suggestion to consider.
@BMurri This is complete and ready for review, though I'm trying to figure out why, when running with this image, I get tasks failing in Batch with ExitCode 10 any time I have more than one running at once. |
Updates and rebase
Address non-Terra concerns for Terra mitigations
Merge with main, resolving conflicts
I welcome C# style feedback, since I'm new to it.
This change adds two new config inputs: Sam URL and billing profile id. If they're provided, the batch scheduler uses these to fetch a distinct managed identity to use to pull the Docker image for the task. This identity is passed down to the TES Runner. Changes elsewhere will ensure that this identity is assigned to the BatchPool. If it isn't, the log contains a useful error message about the identity not being assigned.