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

submitter_guide: add sections on issues and incidents #426

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NikkyXO
Copy link
Contributor

@NikkyXO NikkyXO commented Apr 4, 2023

No description provided.

@NikkyXO
Copy link
Contributor Author

NikkyXO commented Apr 4, 2023

please review added sections @spbnick, am i on right track

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

This is a good start, you get the idea of giving a limited description, which is easier to grasp. Apart from the inline comments, it would also be good to give an example of value for each field.


##### `culprit`
The layers of the execution stack responsible for the issue
It can be one of the following properties
Copy link
Collaborator

Choose a reason for hiding this comment

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

"It can contain one or more of the following properties" would be more correct.


* `Code` - the built or tested code, value is boolean
* `Tool` - the static analyzer, the build toolchain, the test, etc
* `Harness` - the system controlling the execution of the build.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each of these properties is all-lower-case, and should be listed as such, as anything else won't be accepted by the schema. All these properties are boolean, so perhaps just mention that in the paragraph above.

* `FAIL` - the test has failed, the tested code is faulty.
* `PASS` - the test has passed, the tested code is correct.
* `DONE` - the test has finished successfully, the status of the tested code
is unknown.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an incomplete list of possible status strings. It might be better to just refer to the description of the test status field above, as they're the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spbnick i reffered to the test:status section, the available status strings there are: Error, Fail, Pass, Done, Skip. That was what i also included

* `Tool` - the static analyzer, the build toolchain, the test, etc
* `Harness` - the system controlling the execution of the build.

#### Incidents
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use a general description of what these objects are about and for.

@@ -312,6 +312,41 @@ Date/Time Format][datetime_format].

Example: `"2020-08-14T23:41:54+00:00"`

#### Issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use a general description of what these objects are about and for.


##### `origin`
The name of the CI system which submitted the incident

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is missing the crucial fields linking issues and builds/tests together.

@NikkyXO NikkyXO force-pushed the update_submitter_guide_to_use_v4.1_schema-v2 branch from 9d43a8b to 1f98d93 Compare April 16, 2023 16:31
@NikkyXO NikkyXO requested a review from spbnick April 16, 2023 16:36
@NikkyXO
Copy link
Contributor Author

NikkyXO commented Apr 16, 2023

This is a good start, you get the idea of giving a limited description, which is easier to grasp. Apart from the inline comments, it would also be good to give an example of value for each field.

I made recommended changes @spbnick

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Thank you, @NikkyXO. I left a few inline comments concerning your new changes. There are also some of the old ones to resolve.

##### `build_valid`
The status to assign to incident builds

Example: `"build_valid": false`
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's enough to just show the value in the example, like the existing examples for all other properties. No need to also show the property name.

I.e. this:

Example: `false`

And not this:

Example: `"build_valid": false`

Here and everywhere else.


#### Incidents
The following properties are used to describe an issue occurrence or absence of it
=======
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line shouldn't be here, as we're not formatting a title.

##### `comment`
A human-readable comment regarding the incident

Example: `"comment": "<An human readable comment in string>"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make up a comment that would be suitable for an incident?




=======
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line and the extra whitespace above shouldn't be here. Just one blank line would be enough.

A human-readable comment regarding the incident



Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one blank line would be more appropriate here.

submitter_guide: add sections on issues and incidents
@NikkyXO NikkyXO force-pushed the update_submitter_guide_to_use_v4.1_schema-v2 branch from 1f98d93 to bdc73d6 Compare April 28, 2023 13:13
@NikkyXO NikkyXO requested a review from spbnick April 28, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants