[GitHub] [buildstream] abderrahim commented on pull request #1436: Create a dummy sandbox when the sandbox config can't be satisfied

[GitBox <gi...@apache.org>]

abderrahim commented on pull request #1436:
URL: https://github.com/apache/buildstream/pull/1436#issuecomment-1048092359


   > I'm not particularly fond of this patch and how the Platform is digging quite deep into the Sandbox interfaces instead of letting Sandbox handle that bit, although this seems to already be a bit messy before the patch.
   
   Yeah, it is messy. I want to refactor it, but I'd rather have a small patch that can be backported to bst-1 at first.
   
   > Looking at this, my main concern is that we are no longer checking the sandbox config when creating a sandbox in Element, so this could cause some sort of regression in error reporting when actually trying to run commands in the sandbox where the config is not supported.
   
   Checking the sandbox config was merged into `create_sandbox`, since it was only ever called just before creating the sandbox (this is the first commit).
   
   > It seems to me (and I might very well be missing something) that we need to have some semantic in Element.__sandbox() to dictate whether we need the sandbox for the purpose of running commands or not, in order to determine if we need to run this check or not.
   
   What this MR does is to make the check not raise an error, but instead return a dummy sandbox which will raise an error when something tries to use it. This is what the old code was supposed to do, but got broken at some point.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org