Skip to content

Commit afdfbda

Browse files
author
bootc-dev Bot
committed
Sync common files from infra repository
Synchronized from bootc-dev/infra@90f71f8. Signed-off-by: bootc-dev Bot <bot@bootc.dev>
1 parent a19b1a9 commit afdfbda

5 files changed

Lines changed: 212 additions & 24 deletions

File tree

.bootc-dev-infra-commit.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
55772cd1ed6efa2f315f6a1cb03b80c575037932
1+
90f71f83e26a74ba93f130c779f70687a2262ae1

AGENTS.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,10 @@ the commit message text.
5757
## Code guidelines
5858

5959
The [REVIEW.md](REVIEW.md) file describes expectations around
60-
testing, code quality, commit messages, commit organization, etc. If you're
60+
testing, code quality, commit messages, commit organization, etc.
61+
Language-specific guidelines are in
62+
[REVIEW_RUST.md](REVIEW_RUST.md) and
63+
[REVIEW_GOLANG.md](REVIEW_GOLANG.md). If you're
6164
creating a change, it is strongly encouraged after each
6265
commit and especially when the agent thinks a task is complete
6366
to spawn a subagent to perform a review using guidelines (alongside

REVIEW.md

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ easy to generate a *lot* of code for unit tests unnecessarily).
2626
### Separating Parsing from I/O
2727

2828
A recurring theme is structuring code for testability. Split parsers from data
29-
reading: have the parser accept a `&str`, then have a separate function that
30-
reads from disk and calls the parser. This makes unit testing straightforward
31-
without filesystem dependencies.
29+
reading: have the parser accept the raw data (e.g. a string), then have a
30+
separate function that reads from disk and calls the parser. This makes unit
31+
testing straightforward without filesystem dependencies. See the
32+
language-specific review guides for concrete examples.
3233

3334
### Test Assertions
3435

@@ -48,9 +49,7 @@ or `sed`.
4849

4950
Try to avoid having shell script longer than 50 lines. This commonly occurs
5051
in build system and tests. For the build system, usually there's higher
51-
level ways to structure things (Justfile e.g.) and several of our projects
52-
use the `cargo xtask` pattern to put arbitrary "glue" code in Rust using
53-
the `xshell` crate to keep it easy to run external commands.
52+
level ways to structure things (Justfile e.g.).
5453

5554
### Constants and Magic Values
5655

@@ -64,10 +63,10 @@ value was chosen.
6463

6564
### Don't ignore (swallow) errors
6665

67-
Avoid the `if let Ok(v) = ... { }` in Rust, or `foo 2>/dev/null || true`
68-
pattern in shell script by default. Most errors should be propagated by
69-
default. If not, it's usually appropriate to at least log error messages
70-
at a `tracing::debug!` or equivalent level.
66+
Avoid swallowing errors (e.g. `foo 2>/dev/null || true` in shell script).
67+
Most errors should be propagated by default. If not, it's usually appropriate
68+
to at least log error messages at a debug level. See the language-specific
69+
review guides for concrete anti-patterns.
7170

7271
Handle edge cases explicitly: missing data, malformed input, offline systems.
7372
Error messages should provide clear context for diagnosis.
@@ -192,25 +191,28 @@ functionality, ensure equivalent coverage exists.
192191

193192
When multiple contributors co-author a PR, bring in an independent reviewer.
194193

195-
## Rust-Specific Guidance
194+
## Dependencies
196195

197-
Prefer rustix over `libc`. All `unsafe` code must be very carefully
198-
justified.
196+
New dependencies should be justified. Consider alternatives: "I'm curious if
197+
you did any comparative analysis at all with alternatives?"
199198

200-
### Dependencies
199+
Prefer well-maintained libraries with active communities. Glance at existing
200+
reverse dependencies to gauge adoption (e.g. on crates.io for Rust, or
201+
pkg.go.dev for Go). Consider project-level dependency policies (e.g.
202+
`cargo deny` for Rust).
201203

202-
New dependencies should be justified. Glance at existing reverse dependencies
203-
on crates.io to see if a crate is widely used. Consider alternatives: "I'm
204-
curious if you did any comparative analysis at all with alternatives?"
205-
206-
Prefer well-maintained crates with active communities. Consider `cargo deny`
207-
policies when adding dependencies.
208-
209-
### API Design
204+
## API Design
210205

211206
When adding new commands or options, think about machine-readable output early.
212207
JSON is generally preferred for that.
213208

214209
Keep helper functions in appropriate modules. Move command output formatting
215210
close to the CLI layer, keeping core logic functions focused on their primary
216211
purpose.
212+
213+
## Language-Specific Guidance
214+
215+
The following guides cover language-specific review expectations:
216+
217+
- [REVIEW_RUST.md](REVIEW_RUST.md) — Rust projects
218+
- [REVIEW_GOLANG.md](REVIEW_GOLANG.md) — Go projects

REVIEW_GOLANG.md

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
# Go-Specific Review Guidelines
2+
3+
These guidelines supplement the general [REVIEW.md](REVIEW.md) with
4+
Go-specific expectations.
5+
6+
## Separating Parsing from I/O
7+
8+
Have the parser accept a string or `io.Reader`, then have a separate function
9+
that opens the file and calls the parser:
10+
11+
```go
12+
// ✅ Good: parser is a pure function, easy to unit test
13+
func parseConfig(data string) (*Config, error) { ... }
14+
15+
func loadConfig(path string) (*Config, error) {
16+
data, err := os.ReadFile(path)
17+
if err != nil {
18+
return nil, err
19+
}
20+
return parseConfig(string(data))
21+
}
22+
```
23+
24+
## Don't Ignore (Swallow) Errors
25+
26+
Avoid discarding errors with the blank identifier. Most errors should be
27+
returned to the caller. If not, at least log the error:
28+
29+
```go
30+
// ❌ Avoid: error is silently swallowed
31+
_ = doSomething()
32+
33+
// ✅ Good: propagate
34+
if err := doSomething(); err != nil {
35+
return err
36+
}
37+
38+
// ✅ OK if the error is truly ignorable: log it
39+
if err := doSomething(); err != nil {
40+
log.Debug("ignoring error", "err", err)
41+
}
42+
```
43+
44+
## Gomega and Test Assertions
45+
46+
We use [gomega](https://github.com/onsi/gomega) for test assertions. Follow
47+
these conventions:
48+
49+
### Use `g.Eventually` for Polling
50+
51+
Gomega's `Eventually` handles polling, timeouts, and failure reporting.
52+
53+
### Return `(T, error)` from `Eventually` Callbacks
54+
55+
Return the specific field you care about and let gomega matchers describe the
56+
expectation declaratively. This produces better failure messages because
57+
gomega can show what the value actually was vs. what was expected.
58+
59+
```go
60+
// ✅ Good: return the field, match with gomega
61+
g.Eventually(func() ([]metav1.Condition, error) {
62+
var p bootcv1alpha1.BootcNodePool
63+
err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &p)
64+
return p.Status.Conditions, err
65+
}).Should(ContainElement(And(
66+
HaveField("Type", bootcv1alpha1.PoolDegraded),
67+
HaveField("Status", metav1.ConditionTrue),
68+
HaveField("Reason", bootcv1alpha1.PoolNodeDegraded),
69+
)))
70+
71+
// ❌ Avoid: assertions inside the callback with Succeed()
72+
g.Eventually(func(g Gomega) {
73+
var p bootcv1alpha1.BootcNodePool
74+
g.Expect(k8sClient.Get(ctx, ...)).To(Succeed())
75+
cond := apimeta.FindStatusCondition(p.Status.Conditions, ...)
76+
g.Expect(cond).NotTo(BeNil())
77+
g.Expect(cond.Status).To(Equal(...))
78+
}).Should(Succeed())
79+
```
80+
81+
### Return the Narrowest Type
82+
83+
Extract exactly the field you want to assert on — labels, conditions,
84+
ownerReference — rather than returning the whole object or a `bool`:
85+
86+
```go
87+
// Labels
88+
g.Eventually(func() (map[string]string, error) {
89+
var n corev1.Node
90+
err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &n)
91+
return n.Labels, err
92+
}).Should(HaveKey(bootcv1alpha1.LabelManaged))
93+
94+
// OwnerReference
95+
g.Eventually(func() (*metav1.OwnerReference, error) {
96+
var bn bootcv1alpha1.BootcNode
97+
err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, &bn)
98+
return metav1.GetControllerOf(&bn), err
99+
}).Should(And(Not(BeNil()), HaveField("Name", pool.Name)))
100+
```
101+
102+
### Use Composed Matchers for Struct Assertions
103+
104+
Prefer `HaveField` and `ContainElement(And(...))` to match on struct fields
105+
declaratively rather than manually extracting fields and asserting one by one:
106+
107+
```go
108+
// ✅ Good: declarative, one expression
109+
g.Expect(conditions).To(ContainElement(And(
110+
HaveField("Type", bootcv1alpha1.PoolDegraded),
111+
HaveField("Status", metav1.ConditionTrue),
112+
HaveField("Reason", bootcv1alpha1.PoolInvalidSpec),
113+
)))
114+
115+
// ❌ Avoid: manual lookup + sequential field assertions
116+
cond := apimeta.FindStatusCondition(conditions, bootcv1alpha1.PoolDegraded)
117+
g.Expect(cond).NotTo(BeNil())
118+
g.Expect(cond.Status).To(Equal(metav1.ConditionTrue))
119+
g.Expect(cond.Reason).To(Equal(bootcv1alpha1.PoolInvalidSpec))
120+
```
121+
122+
### Assert Specific Errors When Expected
123+
124+
When a test expects a particular error, match on the concrete error type or
125+
value:
126+
127+
```go
128+
// ✅ Good: we know the API server should reject this
129+
g.Expect(err).To(MatchError(apierrors.IsInvalid, "IsInvalid"))
130+
```

REVIEW_RUST.md

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# Rust-Specific Review Guidelines
2+
3+
These guidelines supplement the general [REVIEW.md](REVIEW.md) with
4+
Rust-specific expectations.
5+
6+
## Separating Parsing from I/O
7+
8+
Have the parser accept a `&str`, then have a separate function that reads from
9+
disk and calls the parser:
10+
11+
```rust
12+
// ✅ Good: parser is a pure function, easy to unit test
13+
fn parse_config(data: &str) -> Result<Config> { ... }
14+
15+
fn load_config(path: &Path) -> Result<Config> {
16+
let data = std::fs::read_to_string(path)?;
17+
parse_config(&data)
18+
}
19+
```
20+
21+
## Don't Ignore (Swallow) Errors
22+
23+
Avoid the `if let Ok(v) = ... { }` pattern which silently discards the error
24+
branch. Most errors should be propagated with `?`. If not, at least log the
25+
error:
26+
27+
```rust
28+
// ❌ Avoid: error is silently swallowed
29+
if let Ok(v) = do_something() {
30+
use_value(v);
31+
}
32+
33+
// ✅ Good: propagate
34+
let v = do_something()?;
35+
36+
// ✅ OK if the error is truly ignorable: log it
37+
match do_something() {
38+
Ok(v) => use_value(v),
39+
Err(e) => tracing::debug!("ignoring error: {e}"),
40+
}
41+
```
42+
43+
## Shell Scripts
44+
45+
Several of our projects use the `cargo xtask` pattern to put arbitrary "glue"
46+
code in Rust using the `xshell` crate to keep it easy to run external commands.
47+
This is preferred over long shell scripts.
48+
49+
## General
50+
51+
Prefer rustix over `libc`. All `unsafe` code must be very carefully
52+
justified.
53+

0 commit comments

Comments
 (0)