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

Update possible certification status values for Job units and modify its default (new) #1204

Merged
merged 11 commits into from
Sep 3, 2024

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented Apr 23, 2024

Description

Until now, a Job unit could have 4 possible values for certification status (unspecified, not-part-of-certification, blocker, non-blocker). This PR reduces the number to 2 (blocker and non-blocker), and set the default to non-blocker (instead of unspecified in the past).

Resolved issues

CER-2586

Documentation

  • Job unit reference page is updated
  • Examples making use of unspecified certification values are updated throughout the docs
  • Checkbox submission JSON schema updated

Tests

The disk-cert-automated nested part has a mix of default and cert-blockers.

Running checkbox-cli list-bootstrapped -f "{id} ({certification_status})\n" "com.canonical.certification::disk-cert-automated" allows to see the difference before and after the patch.

Before:

executable (unspecified)
disk/detect (blocker)
disk/stats_dm-1 (unspecified)
disk/read_performance_dm-1 (blocker)
disk/storage_device_dm-1 (blocker)
benchmarks/disk/hdparm-read_dm-1 (unspecified)
benchmarks/disk/hdparm-cache-read_dm-1 (unspecified)
disk/apste_support_on_nvme0 (unspecified)
disk/apste_support_on_nvme1 (unspecified)
manifest (unspecified)
disk/check-software-raid (unspecified)

After:

executable (non-blocker)
disk/detect (blocker)
disk/stats_dm-1 (non-blocker)
disk/read_performance_dm-1 (blocker)
disk/storage_device_dm-1 (blocker)
benchmarks/disk/hdparm-read_dm-1 (non-blocker)
benchmarks/disk/hdparm-cache-read_dm-1 (non-blocker)
disk/apste_support_on_nvme0 (non-blocker)
disk/apste_support_on_nvme1 (non-blocker)
manifest (non-blocker)
disk/check-software-raid (non-blocker)

I also ran this test plan before and after the patch and uploaded the results to C3.

Before:

image

After:

image

pieqq added 6 commits April 23, 2024 14:06
Until now, the default was "unspecified". Set it to "non-blocker". In
addition, remove both "unspecified" and "non-part-of-certification"
statuses, since one is redundant and the other has never been used in
practice.

Fix CER-2586
Since there are only two possible certification statuses now, remove
tests that are not required anymore.
Until now, jobs with an "unspecified" certification status would not
show any certification status in the HTML report. Since "non-blocker"
becomes the default, the same behavior is applied. Therefore, only jobs
with a "blocker" certification status will specify their certification
status. "non-blocker" is assumed for all the others.
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 43.37%. Comparing base (84878c2) to head (0892f75).
Report is 283 commits behind head on main.

Files with missing lines Patch % Lines
checkbox-ng/plainbox/impl/exporter/xlsx.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
+ Coverage   43.21%   43.37%   +0.16%     
==========================================
  Files         356      356              
  Lines       38662    38660       -2     
  Branches     6561     6561              
==========================================
+ Hits        16706    16768      +62     
+ Misses      21293    21213      -80     
- Partials      663      679      +16     
Flag Coverage Δ
checkbox-ng 67.98% <88.88%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pieqq pieqq force-pushed the CER-2586-two-cert-status-only branch from 3694b31 to 427414a Compare April 23, 2024 08:46
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

+1 on the change, less random statusses for jobs is better! A few questions, the most important one is: this doesn't change anything in the remote codebase (as far as I can see). Is this change propagated there somehow I can't understand right now? If so, how?

Minor other questions below.

@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Jul 16, 2024
@pieqq pieqq changed the title Update possible certification status values for Job units and modify its default (Breaking) Update possible certification status values for Job units and modify its default (bugfix) Sep 3, 2024
@pieqq pieqq changed the title Update possible certification status values for Job units and modify its default (bugfix) Update possible certification status values for Job units and modify its default (new) Sep 3, 2024
@pieqq pieqq merged commit 6937537 into main Sep 3, 2024
32 of 33 checks passed
@pieqq pieqq deleted the CER-2586-two-cert-status-only branch September 3, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes The review has been completed but the PR is waiting for changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants