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.

Sign up for GitHub

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

Draft CI requirements #61

Closed
wants to merge 2 commits into from
Closed

Draft CI requirements #61

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c966ee7
draft CI requirements
wusatosi Nov 15, 2024
e3c7d3b
typo fix
wusatosi Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/BEMAN_STANDARD.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,3 +422,21 @@ copyright notice following the SPDX license identifier.

**[CPP.NAMESPACE]** RECOMMENDATION: Headers in `include/beman/<short_name>/` should export
entities in the `beman::<short_name>` namespace.

## Continous Integration
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Continous Integration
## Continuous integration


**[CI.PLATFORM]** REQUIREMENT: All CI should be implemented using GitHub Actions or an CI platform that appripriatly interacts with the code control system.
Copy link
Member Author

@wusatosi wusatosi Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

an CI platform that appropriately interacts with the code control system may need better wording here. My intent here is to say "GitHub Actions is preferred, but if something checks PR, commits and write a check status to each commit and is reasonable, it is acceptable"

Copy link
Member

Choose a reason for hiding this comment

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

If someone wants to pay for an external CI system, that might be acceptable? We can cross that bridge if and when.

But for beman standards purposes, configuration as code is important, and that code should live with the project. That's built in with GH Actions, but it does mean I can mostly coax Gitea to use them, too. I haven't tried Gitlab.

Internally, we use GHE, but not Actions, and instead use our own systems for CI and CD, but they're sufficiently agnostic that it's not an issue. It is why I prefer as minimal magic in the actions as possible, though, so it's straightforward to run tests on change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree, we need to come up with a wording for allowing acceptable external CI or make this item a recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**[CI.PLATFORM]** REQUIREMENT: All CI should be implemented using GitHub Actions or an CI platform that appripriatly interacts with the code control system.
**[CI.PLATFORM]** REQUIREMENT: The CI pipeline must be implemented using GitHub Actions.
  • check REQUIREMENT and "must" in this docs
  • I don't think we need to have external cases for start
  • We should pick between REQUIREMENT: The CI pipeline must be implemented using GitHub Actions. and `RECOMMANDATION: The CI pipeline should be implemented using GitHub Actions. These options are consistent with current doc wording.


**[CI.TEST]** REQUIREMENT: CI must execute appropriate testing of reasonable scope tagged with each commit, and must fail with test cases.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**[CI.TEST]** REQUIREMENT: CI must execute appropriate testing of reasonable scope tagged with each commit, and must fail with test cases.
**[CI.ENABLE_TESTS]** REQUIREMENT: The CI must execute tests for each commit in each PR and must provide explicit output on failure.

and I would remove


**[CI.TEST.CROSS_PLATFORM]** REQUIREMENT: CI enabled tests must be executed across major platforms if appripriate.
Copy link
Member

Choose a reason for hiding this comment

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

We don't have multiple dot usage in the name of the rules in this doc!

  • remove CI.TEST.CROSS_COMPILER_FAMILIES and CI.TEST.CROSS_COMPILER_VERSIONS
Suggested change
**[CI.TEST.CROSS_PLATFORM]** REQUIREMENT: CI enabled tests must be executed across major platforms if appripriate.
**[CI.USE_CROSS_PLATFORM_TESTS]** RECOMMANDATION: The CI tests should be executed across major platforms and compilers.


**[CI.TEST.CROSS_COMPILER_FAMILIES]** REQUIREMENT: CI enabled tests must be executed across major compiler families if appripriate.

Copy link
Member

Choose a reason for hiding this comment

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

These are listed as Requirements, but then come with the conditional 'if appropriate' -- which kinda makes them recommendations I'd say.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fair.

My thinking here is, let's say we have a feature that relies on trunk version of gcc/ clang, this requirement could be waived.

**[CI.TEST.CROSS_COMPILER_VERSIONS]** RECOMMENDATION: CI enabled tests should be executed across support major versions of appripriate compiler families.

**[CI.DURATION_PER_COMMIT]** RECOMMENDATION: CI pipeline associated with commits should finish in a reasonable amount of time.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we include a quantifiable "reasonable amount of time" as suggestion here?

Copy link
Member Author

Choose a reason for hiding this comment

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

My recommendation is 10 mins for small projects, no longer than 30 mins.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**[CI.DURATION_PER_COMMIT]** RECOMMENDATION: CI pipeline associated with commits should finish in a reasonable amount of time.
**[CI.EXECUTION_TIME]** RECOMMENDATION: The entire CI pipeline should run as fast as possible.

Again, I would make it short or remote it.


**[CI.SCHEDULED_TEST]** RECOMMENDATION: CI enabled checks should run periodically across the entire repository.
Copy link
Member

Choose a reason for hiding this comment

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

I would not put this rule in the current PR, as we don't have yet implemented in any repo. Please remove for now.


**[CI.SCEDULED_TEST_NOTIFY_FAILURE]** REQUIREMENT: There must be a established protocol to report CI failure (e.g. opening a GitHub issue) when scheduled CI fails.
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternative wording: A CI run must not fail silently.

Copy link
Member

Choose a reason for hiding this comment

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

I probably don't want an issue opened every time there's a failure...

Copy link
Member

Choose a reason for hiding this comment

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

While it's very useful to know the state of success across compilers, I think it would be fine for say, AppleClang, to not be able to compile something because it's too old to have some feature that's needed, or even just desired, for the library component.

Requiring C++26 is OK. In fact it may be necessary for adoption of some proposals.

Copy link
Member

Choose a reason for hiding this comment

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

I would not put this rule in the current PR, as we don't have yet implemented in any repo. Please remove for now.