Skip to content

Commit

Permalink
verifypr: update check and docs (#447)
Browse files Browse the repository at this point in the history
Updates the `verifypr` to validate feature_set tag, also update docs and PR template.

category: misc
ticket: #381
  • Loading branch information
corverroos committed Apr 27, 2022
1 parent 8d20834 commit 4fd3773
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 3 deletions.
2 changes: 2 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- 2️⃣ PR body: Replace 'body...' with detailed description of the change.
- 3️⃣ category: pick one, delete the rest.
- 4️⃣ ticket: Replace #000 with link to a GitHub issue (or 'none' if PR is trivial).
- 5️⃣ feature_set: pick one (or delete completely if not applicable).
🧑‍🎓 Please review our contribution guide https://github.com/ObolNetwork/charon/blob/main/docs/contributing.md
- 📜 Sign the Contributor License Agreement (CLA) when prompted.
- 🌱 Starting with an issue, outlining the problem and proposed solution, is highly encouraged.
Expand All @@ -15,3 +16,4 @@ body...

category: bug feature refactor docs test fixbuild misc
ticket: #000
feature_set: alpha beta stable
2 changes: 1 addition & 1 deletion docs/branching.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ We follow [Trunk Based Development](https://trunkbaseddevelopment.com/) as a bra

- Since a feature cannot be added as a single big merge of a big feature branch, tools and patterns are required that allow gradual controlled introduction of increment changes without breaking.
- New code can be added as “dead code”. So not integrated into the actual program yet. Once it is properly complete, it can be integrated in a single PR.
- Some features should however not be enabled straight into prod/mainnet, but should be tested in dev/devnet or staging/testnet first. This can be achieved by simple [feature switches](https://trunkbaseddevelopment.com/feature-flags/) (if statements) that integrate new features based on the environment (e.g. only enable feature X in testnet). TODO(corver): Add feature switch functionality.
- Some features should however not be enabled straight into prod/mainnet, but should be rolled-out slowly being first tested in `alpha` (internal devnet only), then `beta` (internal and external testnet), and then only `stable` (enabled everywhere). This can be achieved by simple [feature switches](https://trunkbaseddevelopment.com/feature-flags/) (if statements) that enable features based on their `feature_set` status.
- Another powerful pattern to gradually introduce change is [branching by abstraction](https://trunkbaseddevelopment.com/branch-by-abstraction/). This basically introduces an abstraction layer at the point where a new feature has to replace an old feature (like an interface). Using dependency injection, the new feature can be integrated during testing/staging while the old feature is still being used in production.
- Note that both feature switches and/or abstraction layers used to roll out a feature should be removed once released to prod/main-net.

Expand Down
4 changes: 2 additions & 2 deletions docs/contributing.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ Note: PRs can only be merged by obol-bulldozer bot. It is author's responsibilit
- Ends with a list of tags (some required, others optional) (`^tag: value of this tag\n`):
- `category`: required; one of: `refactor`, `bug`, `feature`, `docs`, `release`, `tidy`, `fixbuild`.
- `ticket`: required; URL of the Github issue just a reference, E.g. `#123` or `none`.
- `phase`: optional; identifies the highest release cycle phase targeted by the change; `aplha`, `beta`, `gold`
- `feature_set`: optional; identifies the highest rollout status targeted by the change; `alpha`, `beta`, `stable`
- Example:
```
runner/tracer: add jaeger otel exporter
Expand All @@ -94,7 +94,7 @@ Adds the jaeger exporter to our opentelemetery infra.
category: feature
ticket: #206
phase: gold
feature_set: stable
```

### Dev tools, git hooks and linters.
Expand Down
26 changes: 26 additions & 0 deletions testutil/verifypr/verifypr.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ func verifyBody(body string) error {
prevLineEmpty bool
foundCategory bool
foundTicket bool
foundFeature bool
)
for i, line := range strings.Split(body, "\n") {
if i == 0 && strings.TrimSpace(line) == "" {
Expand Down Expand Up @@ -204,6 +205,31 @@ func verifyBody(body string) error {
foundTicket = true
}

const featureTag = "feature_set:"
if strings.HasPrefix(line, featureTag) {
if foundFeature {
return errors.New("multiple ticket tag lines")
}

set := strings.TrimSpace(strings.TrimPrefix(line, featureTag))

if set == "" {
return errors.New("feature_set tag empty")
}

sets := map[string]bool{
"alpha": true,
"beta": true,
"stable": true,
}

if !sets[set] {
return errors.New("invalid feature_set tag not one of: alpha, beta, stable")
}

foundFeature = true
}

prevLineEmpty = strings.TrimSpace(line) == ""
}

Expand Down
16 changes: 16 additions & 0 deletions testutil/verifypr/verifypr_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,22 @@ func TestBody(t *testing.T) {
Body: "<!--\n\ncategory: bug\nticket: none",
Error: "instructions not deleted (markdown comments present)",
},
{
Body: "Foo\n\ncategory: bug\nticket: none\nfeature_set: alpha",
Error: "",
},
{
Body: "Foo\n\ncategory: bug\nticket: none\nfeature_set: beta",
Error: "",
},
{
Body: "Foo\n\ncategory: bug\nticket: none\nfeature_set: stable",
Error: "",
},
{
Body: "Foo\n\ncategory: bug\nticket: none\nfeature_set: bad set",
Error: "invalid feature_set tag not one of",
},
}
for i, test := range tests {
t.Run(fmt.Sprint(i), func(t *testing.T) {
Expand Down

0 comments on commit 4fd3773

Please sign in to comment.