Skip to content
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

Build: add a new "Cancelled" final state #9145

Merged
merged 7 commits into from
May 23, 2022
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Apr 27, 2022

This commit introduces a new build final state: "Cancelled".
This state is shown in the build listing page and also in the build details
page. The build will end up on this state when:

  • The user cancels the build by clicking on the button
  • The build is cancelled due to inactivity
  • The build is cancelled because it was duplicated

Ther are a lot of changes required for this:

  • HTML template logic for build details and listing pages
  • API changes to add a new value
  • Knockout.js logic for build details page
  • Build model changes to support a new state
  • Update different places of core logic to change checking for finished state
    to BUILD_FINAL_STATES instead
  • Documentation changes to mention the new state
  • Celery build handler after_return to decide if it's finished or
    cancelled the ending state
  • Logic to decide if the build should be killed due to inactivity

UI examples

Build ending up in a cancelled state

Screenshot_2022-05-04_13-13-15

Build cancelled by the user immediately

Screenshot_2022-05-04_13-13-42

Build cancelled shown on listing page

Screenshot_2022-05-04_13-13-57

Reference #9100

This commit introduces a new build final state: "Cancelled".
This state is shown in the build listing page and also in the build details
page. The build will end up on this state when:

- The user cancels the build by clicking on the button
- The build is cancelled due to inactivity
- The build is cancelled because it was duplicated

Ther are a lot of changes required for this:

 - HTML template logic for build details and listing pages
 - API changes to add a new value
 - Knockout.js logic for build details page
 - `Build` model changes to support a new state
 - Update different places of core logic to change checking for `finished` state
   to `BUILD_FINAL_STATES` instead
 - Documentation changes to mention the new state
 - Celery build handler `after_return` to decide if it's `finished` or
   `cancelled` the ending state
 - Logic to decide if the build should be killed due to inactivity

Reference #9100
@humitos humitos requested review from ericholscher and agjohnson May 4, 2022 11:15
@humitos humitos force-pushed the humitos/build-cancelled-state branch from 124301c to ee3a3a9 Compare May 4, 2022 11:15
@humitos humitos marked this pull request as ready for review May 4, 2022 11:17
@humitos humitos requested review from a team as code owners May 4, 2022 11:17
@ericholscher
Copy link
Member

I think overall this makes sense. I feel like @agjohnson had a bit more context in the discussion thread, so happy to let him take primary reviewer here. Happy to do a technical review if we're 👍 on the direction, if that's useful.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does highlight why I want to centralize this logic. I don't have a great grasp on what could have been missed here, but this looks thorough enough to test.

@@ -138,9 +138,14 @@
{% trans "Build completed" %}
</span>
<span class="build-state-failed"
data-bind="visible: finished() && !success()"
data-bind="visible: finished() && !success() && !cancelled()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the type of logic that I'll move to strictly JS and would like out of templates eventually. For now this is fine though.

@@ -77,6 +77,7 @@ <h3>{% trans "Projects" %}</h3>
<span class="build-state build-state-passing">{% trans "passing" %}</span>
{% else %}
<span class="build-state build-state-failing">{% trans "failing" %}</span>
{# TODO: What whould be show here when "not build.sucess and cancelled"? #}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting rid of this concept in the new templates. There is no singular status that makes sense here in any regard. That is, we are showing the status of the project based on the get_latest_build -- a failure on some minor version can make the entire project look failing.

Instead, the new templates don't show this status at all, but show individual version statuses instead. Even in that case, I don't feel we show the version as "cancelled" though.

@humitos
Copy link
Member Author

humitos commented May 17, 2022

I updated the PR and fixed the tests.

Note that this PR is touching different places and also touches the logic of the builds. It may be worth pinging another member to do a local QA of the different ways to generate the canceled state before deploying this feature. What do you think?

@humitos humitos enabled auto-merge May 23, 2022 07:35
@humitos humitos merged commit 2916317 into main May 23, 2022
@humitos humitos deleted the humitos/build-cancelled-state branch May 23, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants