Update build with "Pulling cache" when downloading the cache #6800
Conversation
There was a problem hiding this comment.
Makes sense -- we should likely set this status during the end stage of the build when we're saving it as well. I found it confusing when waiting on the build that looked done.
I'd love to have a state that is also "Uploading files" or similar, because for the WTD builds, it sits for a minute in that state when it appears the build is already done.
| BUILD_STATE_CLONING = 'cloning' | ||
| BUILD_STATE_INSTALLING = 'installing' | ||
| BUILD_STATE_BUILDING = 'building' | ||
| BUILD_STATE_FINISHED = 'finished' | ||
|
|
||
| BUILD_STATE = ( | ||
| (BUILD_STATE_TRIGGERED, _('Triggered')), | ||
| (BUILD_STATE_PULLING_CACHE, _('Pulling cache')), |
There was a problem hiding this comment.
It would be good to call this something that works for both pushing & pulling, perhaps Updating cache.
There was a problem hiding this comment.
If we want this, we will need to change how we handle the FINISHED status.
Currently, the build is first marked as FINISHED and then the push is performed. The FINISHED status is sent when we leave the context manager of the environment (__exit__) and the cache is pushed after that only if the build was successful.
Taking a look at the code, it seems that our options are:
- update the state of the build to FINISHED outside the environment
__exit__method - use
update_on_success=Falsewhen initializing the environment and update the build status to FINISHED manually after pushing the cache. - push cache always (not only on successful builds)
I think that 2) is our better option since it uses the API we already have in the code to avoid updating the status automatically.
If we are going in this direction, why not to add an specific status for each operation? Pulling/Pushing or Downloading/Uploading or similar.
There was a problem hiding this comment.
I went ahead and implemented 2).
It shows "Pushing cache" if the project has the feature and the build is marked as finished outside the __exit__ method. With these changes, the total build time will count pulling/pushing the cache as well, so it will be more realistic.
I opened a new PR for this at #6810 |
|
@ericholscher I added a new state. It may worth to review these changes as well. |
|
I think we need to think this through a bit more, I'm -1 on the approach of further overloading the concept of build state to accomplish this. A better approach would be to include this in the list of build command output, so that you can see the progression through the stages. It's not as meaningful to keep updating the build state, as this information is lost as soon as the state is updated again. Will the user even ever see this state? With checkouts, we can see the checkout commands progress. This feels like a short term hack to bake in some UX that might not even be needed. I'd rather find a future proof solution and hold off on making any changes to this UX until we know more about how we'll use caching. If feels premature to make this change. |
Show "Uploading" build state when uploading artifacts into storage
|
Agree 👍 |
This is the simplest way to communicate to the user that their builds are already running, but we are pulling down the cache to start from there instead from scratch.
We could discuss other options as well, but I think this is the easiest one and it's a good start for now.
Related to #6793