Expand project guidelines #8314
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Start a new ephemeral environment with changes proposed in this pull request: |
CONTRIBUTING.md
Outdated
| * ... passes PR gating tests, or failing tests are waived by means of explaining the reason. | ||
| * ... adheres to the [coding style](docs/manual/developer/04_style_guide.md#complianceascode-style-guide). | ||
| * ... has been tested. There are following options that allow for testing various aspects of the project: | ||
| * [SSG Test Suite](tests/README.md) for rule tests that ensures that test OVAL checks and that test the ability of remediations to satisfty those checks. |
There was a problem hiding this comment.
| * [SSG Test Suite](tests/README.md) for rule tests that ensures that test OVAL checks and that test the ability of remediations to satisfty those checks. | |
| * [SSG Test Suite](tests/README.md) for rule tests that ensures that test OVAL checks and that test the ability of remediations to satisfy those checks. |
Add measures that will help to keep the code maintenance costs under control and that will make code reviews a smoother experience.
Co-authored-by: Matthew Burket <m@tthewburket.com>
|
@matejak: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
CONTRIBUTING.md
Outdated
| * ... has been approved by an SME of the area that the PR addresses, which indicates that it is generally a step in a right direction. | ||
| * ... passes PR gating tests, or failing tests are waived in a comment by the reviewer explaining the reasoning. | ||
| * ... adheres to the [coding style](docs/manual/developer/04_style_guide.md#complianceascode-style-guide). | ||
| * ... has been tested. The following options that allow for testing various aspects of the project: |
| * [Unit tests](tests/unit) that test components of the build system as well as components of the SSG Test Suite. | ||
| * Ad-hoc tests that are integrated into the `ctest` chain directly, i.e. the shellcheck test. | ||
| * ... updates READMEs, man pages or other documentation when changes of described behavior are introduced. | ||
| * ... doesn't contain merge commits - those can be removed by rebasing. |
There was a problem hiding this comment.
Might be helpful to link to a rebasing/git guide if we have one.
There was a problem hiding this comment.
We don't, please somebody suggest something that is both to the point and isn't located on some obscure blog that may go offline in a couple of years.
There was a problem hiding this comment.
@matejak , this might be useful: https://github.com/marcusburghardt/ansible-role-openscap/blob/master/files/STARTGUIDE.md
It is included in the ansible-role-openscap.
There was a problem hiding this comment.
So for the time being, I would proceed with merging this PR as it is, because your guide doesn't contain all the info that one could be after, and supplying that would take some time. I see no issue with referencing it, or even with putting it here and referencing it from the roles project.
I think that following issues should be covered there:
- merge commits, how to recognize them
- what rebase does, and what can be accomplished by interactive rebase (reordering, merge of commits)
- a few words about conflict resolution - when to expect them and generally how to handle them
There was a problem hiding this comment.
We can also turn on a setting in the project that forbids merge commits to help automate this requirement.
CONTRIBUTING.md
Outdated
| * Ad-hoc tests that are integrated into the `ctest` chain directly, i.e. the shellcheck test. | ||
| * ... updates READMEs, man pages or other documentation when changes of described behavior are introduced. | ||
| * ... doesn't contain merge commits - those can be removed by rebasing. | ||
| * ... is rebased against recent-enough branch that executes the whole range of expected integrated gating tests. |
There was a problem hiding this comment.
nit: Maybe relevant versus expected? The expected behavior, IMO, would be for GitHub to auto-rebase the present target branch state and the proposed PR changes and then run CI on top of that, but sadly it doesn't and instead takes the base branch as-is. :-) Saying relevant would allow for some variation in main branch, but suggest that any tests which impact the proposed changes should be pulled in first. Thoughts?
CONTRIBUTING.md
Outdated
| * ... doesn't contain merge commits - those can be removed by rebasing. | ||
| * ... is rebased against recent-enough branch that executes the whole range of expected integrated gating tests. | ||
| * ... doesn't increase maintenance costs, and if it does, there is a substantial counterbalance. | ||
| * ... doesn't impact the build time in a significant way. |
There was a problem hiding this comment.
Maybe negatively impact? :-) If we improve the build time significantly, that might be good to merge :D
| * Include tests for your contribution. | ||
| * Write comments if and only if there is no chance for the code to explain itself. | ||
| * Don't put authorship information into the code, Git tracks authorship for you. | ||
| Use [SPDX IDs](https://spdx.dev/ids/) for license-related boilerplate in newly written code. |
There was a problem hiding this comment.
Are we suggesting SPDX IDs for all new (code) files or are we saying if a file needs it, we should add it? If the former, should we go back and apply it retroactively? A quick scan of some files in ssg and build-scripts says they're presently lacking, and IMO, I think the LICENSE in the root of the project applies fine enough to avoid SPDX-ing the project.
There was a problem hiding this comment.
Good point, the suggestion's intention was towards "don't copy-paste the whole license".
|
@Xeicker @dodys @anivan-suse any input on this? I tagged you because you did latest contributions for different products. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| * Write comments if and only if there is no chance for the code to explain itself. | ||
| * Don't put authorship information into the code, Git tracks authorship for you. | ||
| Don't copy-paste license text into source files -- use [SPDX IDs](https://spdx.dev/ids/) for that purpose. | ||
| * Don't take part in making files longer - files longer than 400 lines should be an exception. |
There was a problem hiding this comment.
| * Don't take part in making files longer - files longer than 400 lines should be an exception. | |
| * Don't take part in making files longer - files longer than 250 lines should be an exception. |
Currently, Code Climate is set to a 250 LOC limit, we can change that to 400 if you think that is a better limit.
There was a problem hiding this comment.
So I would converge gradually, so let's set the limit to 400, and then restrict it further when there is a lower amount of findings.
There was a problem hiding this comment.
Sounds good; I will get a PR up with the code climate config.
Co-authored-by: Matthew Burket <m@tthewburket.com>
marcusburghardt
merged commit ac402a3
into
ComplianceAsCode:master
yuumasato
added
the
Highlight
marcusburghardt
added
the
governance
Add measures that will help to keep the code maintenance costs under control and that will make code reviews a smoother experience.