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

verifypr: update check and docs #447

Merged
merged 2 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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).
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the cases of non-applicability?
maybe updating github actions would be one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only category: feature requires feature_set I think

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

🧑‍🎓 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; `aplha`, `beta`, `stable`
corverroos marked this conversation as resolved.
Show resolved Hide resolved
- 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