Naming conventions#47
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis pull request adds Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
|
||
| | Concept | Code (Type) | URL/Config | Plural | Database Table | | ||
| |---------|-------------|------------|--------|----------------| | ||
| | Cluster | `Cluster` | `cluster` | `clusters` | `clusters` | |
There was a problem hiding this comment.
I really feel, we should be calling it a hosted cluster, following what we are actually provisioning. I really dont like cluster as it leaves too much room for interpretation
There was a problem hiding this comment.
I can see that for us (dev teams, BU) it may have more meaning using HostedCluster, but..
What would make the most sense to customers?
Does it bring value for them to name it HostedCluster instead of Cluster?
There was a problem hiding this comment.
We are just an API, how it gets wrapped and presented by the client is up to the users. I really worry that if we are not strongly locked to what a hosted cluster is we will fall into the same trap as OCM did
|
|
||
| | Component Type | Suffix | Interface Name | Implementation | Package | | ||
| |----------------|--------|----------------|----------------|---------| | ||
| | Service layer | `Service` | `ClusterService` | `sqlClusterService` | `pkg/services/` | |
There was a problem hiding this comment.
Same as above, really feel we need to be pushing on HostedClusterService
| <summary>Complete Table Schema Example</summary> | ||
|
|
||
| ```sql | ||
| CREATE TABLE clusters ( |
There was a problem hiding this comment.
Off topic, but I noticed in the API we are not using Gorm conventions for creating tables and instead using tx.Exec for raw sql, is there are reason for this?
There was a problem hiding this comment.
IIRC it was because raw SQL will make migrations easier/more controlled than letting gorm doing the updates
|
Nice work @rh-amarin . Although it's a draft, besides the comments from @ciaranRoche regarding using HostedCluster fo examples, this LTGM. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@hyperfleet/docs/naming-conventions.md`:
- Around line 233-234: The broker topic examples use PascalCase (`Cluster`,
`NodePool`) but section 1.1 mandates lowercase in URLs/config; update
naming-conventions.md to remove ambiguity by either (a) changing the example
resource_type values to lowercase (`cluster`, `nodepool`) and updating
`BROKER_TOPIC`/`BROKER_SUBSCRIPTION_ID` examples accordingly, or (b) adding one
clear sentence stating that broker topic names are an explicit exception and
must use PascalCase, referencing the table examples (`Cluster`, `NodePool`,
`BROKER_TOPIC`, `BROKER_SUBSCRIPTION_ID`) so readers know which rule applies;
implement one of these fixes so examples and the "lowercase in config" rule are
consistent.
- Line 148: Update the typo in the sentence "We should standarize in:" by
replacing "standarize" with "standardize" so the line reads "We should
standardize in:"; locate the phrase in the naming-conventions.md content (the
exact text "We should standarize in:") and correct the spelling.
- Around line 354-367: The fenced code block showing the secrets directory tree
lacks a language tag (Markdown lint), so update the triple-backtick fence
surrounding the directory tree to include the text language specifier (i.e.,
change ``` to ```text) so the block is typed; locate the directory-tree fenced
block in naming-conventions.md (the secrets/ tree) and add the language
identifier `text` immediately after the opening backticks.
- Around line 743-747: The fenced code block showing the directory tree (the
block containing "cmd/ └── <service-name>/ └── main.go") lacks a language tag;
update the opening fence to use ```text so the tree is rendered as plain text
(replace the current ``` with ```text for that directory-tree code block).
- Line 739: Change the phrase "Full stack services keep the `hyperfleet-` prefix
(e.g., `hyperfleet-api`)" to use the hyphenated compound adjective "Full-stack
services keep the `hyperfleet-` prefix (e.g., `hyperfleet-api`)" so the compound
modifier is correctly hyphenated; locate the sentence containing "Full stack
services" in the naming-conventions.md content and replace it with "Full-stack
services".
- Around line 665-701: The docs state "use lower case for the CloudEvent type"
but the Field Standards and example show PascalCase
("com.redhat.hyperfleet.Cluster.reconcile"); change the type pattern to use
lowercase resource and action (e.g.,
`com.redhat.hyperfleet.<resource>.<action>`), update the Field Standards entry
for `type` and the CloudEvents JSON example accordingly (replace
`com.redhat.hyperfleet.Cluster.reconcile` with
`com.redhat.hyperfleet.cluster.reconcile`), and ensure the prose and examples
consistently reference the `type` field as lowercase.
- Around line 833-862: The fenced code block displaying the repository directory
tree in naming-conventions.md is missing a language tag; update the
triple-backtick fence that wraps the directory listing to use the text language
(i.e., change ``` to ```text) so the tree is rendered with the correct syntax
highlighting and formatting.
🧹 Nitpick comments (1)
hyperfleet/docs/naming-conventions.md (1)
144-144: Avoid bare URL; use a Markdown link.
Bare URL violates markdownlint and reduces readability.♻️ Suggested edit
-TBC in https://issues.redhat.com/browse/HYPERFLEET-323 +TBC in [HYPERFLEET-323](https://issues.redhat.com/browse/HYPERFLEET-323)
| | Topic name | `<resource_type>` | `Cluster`, `NodePool` | `BROKER_TOPIC` | | ||
| | Queue/Subscription | `<service>-<resource>-sub` | `adapter-cluster-sub`, `sentinel-cluster-sub` | `BROKER_SUBSCRIPTION_ID` | |
There was a problem hiding this comment.
Clarify topic casing vs “lowercase in config” rule.
Section 1.1 says lowercase in URLs/config, but broker topic examples use Cluster/NodePool (PascalCase). Please state explicitly whether broker topic names are an exception or align them to lowercase to avoid divergent implementations.
🤖 Prompt for AI Agents
In `@hyperfleet/docs/naming-conventions.md` around lines 233 - 234, The broker
topic examples use PascalCase (`Cluster`, `NodePool`) but section 1.1 mandates
lowercase in URLs/config; update naming-conventions.md to remove ambiguity by
either (a) changing the example resource_type values to lowercase (`cluster`,
`nodepool`) and updating `BROKER_TOPIC`/`BROKER_SUBSCRIPTION_ID` examples
accordingly, or (b) adding one clear sentence stating that broker topic names
are an explicit exception and must use PascalCase, referencing the table
examples (`Cluster`, `NodePool`, `BROKER_TOPIC`, `BROKER_SUBSCRIPTION_ID`) so
readers know which rule applies; implement one of these fixes so examples and
the "lowercase in config" rule are consistent.
| ``` | ||
| secrets/ | ||
| ├── db.host | ||
| ├── db.name | ||
| ├── db.user | ||
| ├── db.password | ||
| ├── db.port | ||
| ├── hyperfleet-api-token | ||
| ├── ocm.client-id | ||
| ├── ocm.client-secret | ||
| ├── tls.crt | ||
| ├── tls.key | ||
| └── ca.crt | ||
| ``` |
There was a problem hiding this comment.
Add a language to fenced code block.
Markdownlint flags untyped fences; use text for directory trees.
♻️ Suggested edit
-```
+```text
secrets/
├── db.host
├── db.name
├── db.user
├── db.password
├── db.port
├── hyperfleet-api-token
├── ocm.client-id
├── ocm.client-secret
├── tls.crt
├── tls.key
└── ca.crt</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@hyperfleet/docs/naming-conventions.md` around lines 354 - 367, The fenced
code block showing the secrets directory tree lacks a language tag (Markdown
lint), so update the triple-backtick fence surrounding the directory tree to
include the text language specifier (i.e., change ``` to ```text) so the block
is typed; locate the directory-tree fenced block in naming-conventions.md (the
secrets/ tree) and add the language identifier `text` immediately after the
opening backticks.
|
|
||
| **Pattern**: | ||
| - Service-level tools use short names (e.g., `sentinel`) | ||
| - Full stack services keep the `hyperfleet-` prefix (e.g., `hyperfleet-api`) |
There was a problem hiding this comment.
Hyphenate compound adjective: “full-stack”.
Use “full-stack services” when modifying a noun.
✏️ Suggested edit
-- Full stack services keep the `hyperfleet-` prefix (e.g., `hyperfleet-api`)
+- Full-stack services keep the `hyperfleet-` prefix (e.g., `hyperfleet-api`)🧰 Tools
🪛 LanguageTool
[uncategorized] ~739-~739: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ls use short names (e.g., sentinel) - Full stack services keep the hyperfleet- prefix ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
In `@hyperfleet/docs/naming-conventions.md` at line 739, Change the phrase "Full
stack services keep the `hyperfleet-` prefix (e.g., `hyperfleet-api`)" to use
the hyphenated compound adjective "Full-stack services keep the `hyperfleet-`
prefix (e.g., `hyperfleet-api`)" so the compound modifier is correctly
hyphenated; locate the sentence containing "Full stack services" in the
naming-conventions.md content and replace it with "Full-stack services".
| ``` | ||
| cmd/ | ||
| └── <service-name>/ # e.g., sentinel, adapter, hyperfleet-api | ||
| └── main.go | ||
| ``` |
There was a problem hiding this comment.
Add a language to fenced code block.
Use text for directory trees.
♻️ Suggested edit
-```
+```text
cmd/
└── <service-name>/ # e.g., sentinel, adapter, hyperfleet-api
└── main.go</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
743-743: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@hyperfleet/docs/naming-conventions.md` around lines 743 - 747, The fenced
code block showing the directory tree (the block containing "cmd/ └──
<service-name>/ └── main.go") lacks a language tag; update the opening fence to
use ```text so the tree is rendered as plain text (replace the current ``` with
```text for that directory-tree code block).
| ``` | ||
| <repository>/ | ||
| ├── cmd/ # Application entry points | ||
| │ └── <service>/ | ||
| │ └── main.go | ||
| ├── pkg/ # Public packages (importable) | ||
| │ ├── api/ # API models | ||
| │ ├── logger/ # Logging | ||
| │ ├── errors/ # Error types | ||
| │ └── ... | ||
| ├── internal/ # Private packages (not importable) | ||
| │ ├── <component>/ | ||
| │ └── ... | ||
| ├── test/ # Integration tests | ||
| │ └── integration/ | ||
| ├── configs/ # Configuration examples | ||
| ├── charts/ # Helm charts | ||
| │ └── <service>/ | ||
| ├── deployments/ # Deployment artifacts (optional) | ||
| │ ├── helm/ | ||
| │ └── dashboards/ | ||
| ├── docs/ # Documentation | ||
| ├── scripts/ # Build/deployment scripts | ||
| ├── .github/ # GitHub Actions workflows | ||
| ├── Dockerfile | ||
| ├── Makefile | ||
| ├── go.mod | ||
| ├── go.sum | ||
| └── README.md | ||
| ``` |
There was a problem hiding this comment.
Add a language to fenced code block.
Use text for directory layout trees.
♻️ Suggested edit
-```
+```text
<repository>/
├── cmd/ # Application entry points
│ └── <service>/
│ └── main.go
├── pkg/ # Public packages (importable)
│ ├── api/ # API models
│ ├── logger/ # Logging
│ ├── errors/ # Error types
│ └── ...
├── internal/ # Private packages (not importable)
│ ├── <component>/
│ └── ...
├── test/ # Integration tests
│ └── integration/
├── configs/ # Configuration examples
├── charts/ # Helm charts
│ └── <service>/
├── deployments/ # Deployment artifacts (optional)
│ ├── helm/
│ └── dashboards/
├── docs/ # Documentation
├── scripts/ # Build/deployment scripts
├── .github/ # GitHub Actions workflows
├── Dockerfile
├── Makefile
├── go.mod
├── go.sum
└── README.md</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
833-833: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @hyperfleet/docs/naming-conventions.md around lines 833 - 862, The fenced
code block displaying the repository directory tree in naming-conventions.md is
missing a language tag; update the triple-backtick fence that wraps the
directory listing to use the text language (i.e., change totext) so the
tree is rendered with the correct syntax highlighting and formatting.
</details>
<!-- fingerprinting:phantom:triton:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
/retest |
rh-amarin
force-pushed
the
naming-conventions
branch
from
396f78e to
ba5d7c9
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rh-amarin
force-pushed
the
naming-conventions
branch
from
ba5d7c9 to
e3bb7e7
Compare
3 participants
This PR documents how to name different concerns for the different Hyperfleet components.