-
Notifications
You must be signed in to change notification settings - Fork 198
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
project: Support Remote Builds #4161
Conversation
0384a01
to
30e871e
Compare
This change sketches out support for using the ACR remote build feature and uses it the the `containerapp` service target. To use it, add `remoteBuild: true` to the `docker` member of a `service` in `azure.yaml` Fixes Azure#509
30e871e
to
32ffaa5
Compare
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.
This is really neat. As a user, the remote logs being streamed into the terminal was delightful. As a reviewer, the extra care to honor docker ignore semantics felt like great attention to detail.
I just had a small question about how build failures are handled.
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.
I am worried about how this remote build feature depends way too much on overriding the default workflow up to bring build after provision. Should we consider adding a descriptive error about it? Telling users how they need to move build after provision for this to work?
I am approving as looking at the feature only, it looks really good and useful.
Keep in mind to update the azure.yaml schema as part of this or after to avoid the highlighted issue from azure.yaml
@vhvb1989 - I'm not certain I understand your concern here - it is true that enabling the remote build feature means that the Build stage of the docker project is now an "no-op" (as is Let me know if I misunderstood your concern here. |
Instead of tracking spans for both RemoteBuilds and Upload, just track a count of the number of remote builds we did.
362b263
to
d9ec9bd
Compare
This change pulls the status after the build completes and uses that to inform if `deploy` should pass or not.
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.
LGTM, looking forward to getting this feature into the wild and seeing how it's adopted.
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.
This looks great - my only potentially blocking feedback would be around testing with various sized containers / contexts that may take a longer time to upload/build to ensure we are supporting those properly.
When you call the `blockblob.GetProperties`, the azure-sdk-from-go makes a HEAD request to fetch information about the target blob, and uses the result to hydrate the properties object that is returned. The `ContentLength` property (which tells you how big the blob is) is set based on the `Content-Length` header in the response. The type of `ContentLength` is `*int` and if the `Content-Length` header is not set in the response, then the value will remain `nil`. As part of the new ACR remote build feature, to stream logs from the remote build to the console we use `blockblob.GetProperties` to fetch the length of the remote build log file and then do a range request to fetch the content that we have not yet processed. When I wrote an end to end test and tried to record it, this code ended up crashing with a nil reference because the `ContentLength` property was `nil`. This ended up being due to logic in the recording proxy which uses chuncked encoding when returning the response from the proxy server to the client. Per the HTTP spec, when using chunked encoding, the response should not have a Content-Length header, and the indeed the go standard library removes the header before writing the response. Talking with Wei, we think this code to use chunked response was added early on in the development of the proxy to work around some content length mismatches he was facing and may not be needed long term. For now, I've updated the logic to not used chunked encoding for HEAD requests. Since HEAD requests to have a body there's no real value in using chunked responses and doing this ensures the Content-Length header is returned in the response. While in the area, I also updated the middleware that we have to expand gzip'd responses (so that the recording contains the expanded content) to update the Content-Length property when present to be the length of the expanded content instead of just dropping it. While investigating this issue I first thought this was where the problem was, but it ended up being the chunked encoding, the HEAD response returned by Blob Storage is not gzip encoded.
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.
LGTM. Thanks for thanks time to carefully addressing the suggestions!
cea2971
to
3e5c76f
Compare
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This adds the `remoteBuild` configuration knob which was added in Azure#4161 to our schema for `azure.yaml`. Fixes Azure#4353
This change sketches out support for using the ACR remote build
feature and uses it in the
containerapp
service target.To use it, add
remoteBuild: true
to thedocker
member of aservice
inazure.yaml
addresses #509