Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Shinnosuke Sawada-Dazai <3600530+Warashi@users.noreply.github.com>
✅ Deploy Preview for pipecd-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Warashi
changed the title
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Adds explicit PipeCD Go coding and review guidance, linked from contributor documentation.
Changes:
- Adds comprehensive Go coding guidelines.
- Links the guidelines from the versioned contributor guide and
CONTRIBUTING.md.
| File | Description |
|---|---|
docs/content/en/docs-v1.0.x/contribution-guidelines/go-coding-guidelines.md |
New Go coding guidance |
docs/content/en/docs-v1.0.x/contribution-guidelines/contributing-to-pipecd.md |
Links to Go guidelines |
CONTRIBUTING.md |
Links to Go guidelines |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7393 +/- ##
=======================================
Coverage 29.81% 29.81%
=======================================
Files 601 601
Lines 64453 64453
=======================================
Hits 19214 19214
Misses 43746 43746
Partials 1493 1493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Warashi
left a comment
Warashi
left a comment
There was a problem hiding this comment.
I wrote comments around my proposal for coding guidelines; these were also written with AI support. All thoughts are my own, but the representation is AI-supported.
| PipeCD-specific guidance for writing and reviewing Go code. | ||
| --- | ||
|
|
||
| These guidelines aim to make everyday coding decisions explicit so |
There was a problem hiding this comment.
The file is written in its intended post-merge form. It documents
existing practices and proposes explicit defaults where those
practices differ; it is not a claim that every rule is already agreed
upon or consistently implemented. I have kept the proposal rationale
in review comments so it will not become part of the published guide.
The goal is to reduce repeated style discussions and focus reviews on
behavior. General Go guidance is linked by topic rather than copied.
The scope is new and changed code, not a repository-wide cleanup.
Implementation changes and additional lint rules are separate work.
| returns successfully, the SDK should check whether a response is | ||
| present when the contract requires one. | ||
|
|
||
| Return `codes.Internal` for a nil response that the contract does not |
There was a problem hiding this comment.
I propose making response presence an explicit SDK contract and
checking it at the SDK boundary. An unexpected nil response should be
reported as a plugin contract violation, not accepted as success,
replaced with an empty response, or left to fail during conversion.
This preserves intentional nil responses: DetermineStrategy already
documents (nil, nil) and the SDK falls back to PipelineSync. The
proposal is about response presence, not general panic recovery or
validation of every response field. The implementation belongs in a
separate change.
|
|
||
| For PipeCD configuration and plugin contracts: | ||
|
|
||
| - Require a configuration type's `Validate()` method through an |
There was a problem hiding this comment.
I propose expressing required capabilities such as configuration
validation through interfaces or type constraints, rather than
discovering their absence at runtime. pkg/configv1.Spec already
expresses Validate this way.
This is not a ban on runtime type assertions. Initializer is an
optional plugin capability and should remain optional. Required-field
and range validation still belongs at boundaries. Apply this when
designing new or changed code, without a bulk rewrite of existing
contracts.
| Plugin errors or stage logs should provide enough information to | ||
| identify the failed operation and investigate its cause. Record the | ||
| cause even when a normal execution failure is represented by a failed | ||
| stage result rather than an RPC error. |
There was a problem hiding this comment.
The proposed requirement is diagnostic: retain enough information to
identify the failed operation and investigate its cause, including
failures returned as a stage result rather than an RPC error.
I am not proposing a shared error taxonomy, a new wrapper, or
preserving plugin-specific sentinel identity across RPC calls. There
is no automatic error-category-based decision in scope here. If a
concrete consumer needs that distinction, classification should be
designed for that need separately. gRPC status conversion stays at the
SDK boundary; this does not prescribe Internal for every failure.
| Choose synchronization tools according to the required behavior, not | ||
| just for visual consistency. Decide whether one failure cancels the | ||
| other operations, whether all operations must finish, and how their | ||
| results determine the overall result. Reuse an existing helper when it | ||
| provides those same semantics. |
There was a problem hiding this comment.
I propose choosing and reusing fan-out helpers by their failure
semantics, not by a preference for one synchronization primitive.
For example, kubernetes_multicluster's runOnTargets cancels the shared
context on failure and reports overall failure if any target fails.
Its rollback path waits for every target and currently reports
success if at least one result is not StageStatusFailure. Replacing
one with the other would change behavior. This guideline preserves
that distinction; it does not endorse or redesign the rollback
success criterion or require a new generic helper.
|
|
||
| ### Choose test doubles by maintenance cost | ||
|
|
||
| Default to a concise handwritten test double for a small interface |
There was a problem hiding this comment.
I propose a small handwritten test double as the default when it is
concise, while retaining generated mocks when they already fit or
reduce maintenance cost. The decision should follow the interface and
test, not a blanket main-module-versus-plugin rule.
Independent plugins cannot import main-module test helpers, but that
does not prevent generating mocks within a plugin. This proposal does
not migrate existing tests or impose a new fake/mock naming taxonomy.
| In new table-driven tests, use `want` for an expected value and | ||
| `wantErr` for an expected error. When there are multiple expected | ||
| values, add meaning to the name, such as `wantStatus`. |
There was a problem hiding this comment.
I propose want/wantErr, with descriptive suffixes such as wantStatus,
as the default for new test tables. This gives reviewers a predictable
convention without claiming that existing expected/expect-style names
are incorrect.
When adding cases to an existing table, keep that table's naming.
Unrelated renames, a repository-wide migration, and a dedicated linter
are not part of this proposal.
| Within PipeCD's existing testify tests, use `require` for | ||
| prerequisites and `assert` for independent expectations. This is a | ||
| project-specific choice, not adoption of Go Test Comments' | ||
| recommendation to avoid assertion libraries. |
There was a problem hiding this comment.
I propose using require when failure invalidates later checks, and
assert for independent expectations. The deciding factor is whether
the test can meaningfully continue, not whether the assertion happens
to inspect an error.
For example, checking an error before calling err.Error() is a
prerequisite. Blanket require-error enforcement would miss this
distinction, so it is not part of the proposal. References to Go Test
Comments cover continuation, not its recommendation against
assertion libraries.
What this PR does:
This PR adds Go coding guidelines for PipeCD contributors.
Some sections are based on analysis with Copilot, and some are my own ideas.
I'm not using English as usual, so I wrote these guidelines using Copilot. Some descriptions may seem like AI-generated English, and it's right. But all of these are my own themes.
Why we need it:
Happily, we have had many contributors recently. So it's helpful to provide project-specific guidance on Go coding.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: No
Screenshots/Videos (for documentation or website changes):
Attach a screenshot or screen recording if your PR modifies any files under
docs/or any.md/.mdxfiles. Run the site locally usingmake run/siteand verify your changes before submitting.