Add agentic skills for provider review and development - #191
Open
jonasz-lasut wants to merge 3 commits into
Open
Add agentic skills for provider review and development#191jonasz-lasut wants to merge 3 commits into
jonasz-lasut wants to merge 3 commits into
Conversation
Signed-off-by: Jonasz Łasut-Balcerzak <jonasz@upbound.io>
Contributor
Author
|
> ⚠️ Experimental — validate all results against current code before acting.
# Upjet Provider Best Practices Review — `provider-upjet-harbor`
**Target:** `github.com/jonasz-lasut/provider-upjet-harbor` · **Commit:** `2cda927` · **Branch:** `cve-mitigation` · **Reviewed:** 2026-06-24
**Against:** official Upbound `provider-upjet-{aws,azure,gcp}` conventions (`baseline.md`)
---
## Detected Profile
| Axis | Value |
|---|---|
| **Generator** | `github.com/crossplane/upjet/v2` @ `v2.2.1-0.20260414070754-c6d5213346ac` |
| **Backend** | TerraformPluginSDK (`WithTerraformPluginSDKIncludeList` in `config/provider.go:51`) |
| **Crossplane model** | **v2** — dual `apis/cluster/` + `apis/namespaced/`; `crossplane-runtime/v2 v2.2.1`; `.m.` root group confirmed (`harbor.m.crossplane.io`, `config/provider_namespaced.go:21`) |
| **Packaging** | **Monolith-only** — single `cmd/provider/main.go`; 21 resources (well below family threshold of >100) |
| **Build** | **Standard** — `build/` submodule → `github.com/crossplane/build` (`.gitmodules:1-3`); Makefile includes `build/makelib/*.mk` |
Category 5 (v2 dual-scope) **applies**. Category 6 family items are **N/A** — monolith at 21 resources.
---
## Scorecard
| # | Category | MUST | SHOULD | NICE | Status |
|---|----------|------|--------|------|--------|
| 1 | Repo structure & build | 3/3 | 1/1 | — | ✅ |
| 2 | Code generation | 3/3 | 2/2 | 0/1 | ⚠️ |
| 3 | External-name & config | 1/1 | 2/4 | 0/1 | ⚠️ |
| 4 | API types & versioning | 3/3 | 1/2 | — | ⚠️ |
| 5 | v2 scope architecture | 4/4 | — | — | ✅ |
| 6 | Family & packaging | **2/3** | 1/2 | — | ❌ |
| 7 | Controller runtime wiring | 3/3 | 3/3 | 0/2 | ⚠️ |
| 8 | Examples & e2e coverage | 3/3 | 1/1 | 0/2 | ⚠️ |
| 9 | Testing & linting | 2/2 | 3/3 | — | ✅ |
| 10 | CI/CD & governance | 3/3 | 2/2 | 0/1 | ⚠️ |
**e2e coverage:** `20/21` raw ≈ **95%** · gap (`G\E`) = 1 id (`/v1alpha1/replication` — phantom from malformed dependency annotation, see §8) · extra (`E\G`) = 1 (`e2e-full` hand-written bundle) · manual-intervention = 0
---
## Overall Verdict: `NOT-FOLLOWING-BEST-PRACTICES`
One MUST failure gates the verdict: `package/crossplane.yaml` is stripped of all required governance metadata (maintainer, source, license, Crossplane version constraint). Everything else is strong — the dual-scope v2 architecture is correctly and completely wired, `SetupGated`/SafeStart is fully implemented, CI is thorough with `check-diff` and breaking-change guards, e2e coverage is 95%, and the credential-resolution client correctly handles both cluster-scoped and namespaced managed resources. Fixing the package manifest would immediately elevate this to **FOLLOWING-BEST-PRACTICES-WITH-GAPS**, leaving only SHOULD/NICE gaps.
---
## Findings
### 1. Repository Structure & Build — ✅
All standard top-level directories present: `apis/ config/ internal/ cmd/ examples/ examples-generated/ package/ build/ hack/ cluster/test/ .github/`. No `generate/` top-level dir; generation is driven by `apis/generate.go` (valid upjet v2 pattern). `build/` is a git submodule pointing at `https://github.com/crossplane/build` (`.gitmodules:3`). Makefile includes `build/makelib/{common,output,golang,k8s_tools,imagelight,xpkg}.mk`. `go.mod` coherent.
Minor notes (neither MUST nor SHOULD failures):
- Makefile uses `PROJECT_NAME` rather than the `PROVIDER_NAME` mentioned in baseline; crossplane/build makelib uses `PROJECT_NAME` as its primary variable, so this is correct.
- `SUBPACKAGES` not explicitly declared — N/A for monolith; makelib default applies.
- `GO_SUBDIRS += cmd internal apis` (no `generate`) — covered since generation lives in `apis/`.
### 2. Code-generation Pipeline — ⚠️ 1 NICE gap
- ✅ **2.1** `cmd/generator/main.go:24-35`: calls `config.GetProvider()` + `config.GetProviderNamespaced()`, passes both to `pipeline.Run`.
- ✅ **2.2** `apis/generate.go`: orchestrates scraper → upjet generator → `controller-gen` (CRDs + deepcopy) → `angryjet` (method sets) → hand-patch for `*int64` resolver fields (documented inline). Also deletes stale `zz_*` before regeneration.
- ✅ **2.3** All generated files prefixed `zz_`; header `// Code generated by upjet. DO NOT EDIT.` confirmed in `apis/cluster/zz_register.go:5`.
- ✅ **2.4** `config/schema.json` + `config/provider-metadata.yaml` both present.
- ✅ **2.5** Resolver step: `zz_generated.resolvers.go` present in both cluster and namespaced scopes; cross-group reference resolution confirmed.
- **[NICE] ❌ 2.6** — `config/generated.lst` absent from repo (Makefile references it in `generate.init` via `version_diff.py` but it is not committed). Fix: ensure `make generate` writes and commits the file, or make `version_diff.py` tolerant of a missing baseline.
### 3. External-name & Resource Configuration — ⚠️ 2 SHOULD + 1 NICE gap
- ✅ **3.1** `config/external_name.go:11-31`: 21-resource map present and fully populated.
- ✅ **3.3** Per-service `Configure(p *ujconfig.Provider)` in every service package (`config/cluster/<svc>/config.go`). Registry wired via `ProviderConfiguration.AddConfig(...)` in `config/cluster/provider.go` and `config/namespaced/provider.go`. Both cluster and namespaced registries are independent and complete.
- ✅ **3.5** Cross-resource references configured: `harbor_replication`→`harbor_registry` (via `r.References["registry_id"]` in `config/cluster/harbor/config.go:30`), `harbor_project_member_{group,user}`→`harbor_project`+group/user, `harbor_project_webhook`→`harbor_project`.
- **[SHOULD] ⚠️ 3.2** — All 21 resources use `config.IdentifierFromProvider` (`config/external_name.go:13-31`). Harbor assigns numeric server-side IDs for most resources (correct), but `harbor_user` (username field is user-specified), `harbor_label` (name is user-specified), and `harbor_registry` (name) may support `NameAsIdentifier`. Fix: audit against Terraform import documentation; add `NameAsIdentifier` where the user supplies the stable ID.
- **[SHOULD] ⚠️ 3.4** — No `config/overrides.go` or `config/groups.go`. Group shortening is done inline via `flattenHarborShortGroup()` in `config/provider.go:71-78` and `config/provider_namespaced.go`. This is functional and documented but diverges from the `ReplaceGroupWords` convention. Fix: extract into `config/overrides.go` for discoverability.
- **[NICE] ❌ 3.6** — No `config/externalnamenottested.go`. Not critical at 95% coverage.
### 4. API Types & Versioning — ⚠️ 1 SHOULD gap
- ✅ **4.1** Generated `zz_*_types.go`, `zz_*_terraformed.go`, `zz_generated.deepcopy.go`, `zz_generated.managed.go`, `zz_generated.managedlist.go` confirmed in `apis/cluster/harbor/v1alpha1/` and all other service groups. Conversion hubs present (`zz_generated.conversion_hubs.go`) as upjet boilerplate.
- ✅ **4.2** Hand-written ProviderConfig API: `apis/cluster/v1beta1/types.go` + `register.go` + `doc.go`; matching in `apis/namespaced/v1beta1/`.
- ✅ **4.3** `apis/cluster/zz_register.go` and `apis/namespaced/zz_register.go` aggregate all groups into the scheme.
- **[SHOULD] ⚠️ 4.4** — All 21 managed resource types are at `v1alpha1` only (e.g., `apis/cluster/harbor/v1alpha1/`). Convention expects `v1beta1`+ for resources with e2e coverage. 20/21 resources are e2e-tested; promotion to `v1beta1` would signal API stability. Fix: run `make generate` after bumping version in external name config; introduce a `v1alpha1→v1beta1` conversion hub.
### 5. Crossplane v2 Scope Architecture — ✅ All 4 MUST pass
- ✅ **5.1** Dual `apis/cluster/` + `apis/namespaced/` trees, both fully generated.
- ✅ **5.2** Namespaced `.m.` suffix confirmed: `// +groupName=immutable.harbor.m.crossplane.io` (all namespaced `zz_groupversion_info.go` files); root group `harbor.m.crossplane.io` (`config/provider_namespaced.go:21`).
- ✅ **5.3** `config/cluster/` + `config/namespaced/` with independent registries. Both call the same service `Configure` functions through their own `ProviderConfiguration`.
- ✅ **5.4** Two ProviderConfig kinds: cluster-scoped `ProviderConfig` (`apis/cluster/v1beta1/types.go`) + namespaced `ProviderConfig` and `ClusterProviderConfig` (`apis/namespaced/v1beta1/types.go`). Both seeded in `cluster/test/setup.sh`.
### 6. Family Provider & Packaging — ❌ 1 MUST failure
- **[MUST] ❌ 6.1** — `package/crossplane.yaml` is 7 lines: `apiVersion`, `kind`, `metadata.name`, and `spec.capabilities: [SafeStart]` only. Missing: `meta.crossplane.io/maintainer`, `meta.crossplane.io/source`, `meta.crossplane.io/license` annotations and a `spec.crossplane.version` constraint (e.g., `">=v2.0.0-0"`). Evidence: `package/crossplane.yaml` full content. Fix: add the governance annotations and a Crossplane version floor.
- ✅ **6.2** 45 CRDs in `package/crds/`. RBAC is embedded in CRDs via crossplane-runtime annotations (no separate `auth.yaml` is required for upjet providers).
- ✅ **6.3** `SafeStart` capability declared (`package/crossplane.yaml:6`). (This is a SHOULD, counted as pass.)
- N/A **6.4** Family split — monolith with 21 resources is below the >100 threshold.
- **[SHOULD] ⚠️ 6.5** — Publish workflow targets `registry_org: jonasz-lasut` (personal GHCR namespace), while Makefile has `XPKG_REG_ORGS ?= ghcr.io/crossplane-contrib`. The two diverge; for a community provider the canonical target is `xpkg.crossplane.io/crossplane-contrib`. Fix: align `registry_org` with `XPKG_REG_ORGS`.
### 7. Controller Runtime Wiring — ⚠️ All MUST+SHOULD pass; 2 NICE gaps
- ✅ **7.1** `func Setup` in `internal/controller/cluster/zz_setup.go` and `internal/controller/namespaced/zz_setup.go`.
- ✅ **7.4** `func SetupGated` alongside `Setup`; `cmd/provider/main.go:178-189` performs a live RBAC SAR check (`canWatchCRD`) to decide between `SetupGated` (normal) and `Setup` (degraded). More sophisticated than the baseline bootcheck pattern.
- ✅ **7.2–7.3** `internal/clients/harbor.go`: `TerraformSetupBuilder` builds `terraform.Setup` from ProviderConfig; `resolveModern` resolves namespaced `ProviderConfig`/`ClusterProviderConfig` via scheme lookup; `resolveLegacy` handles cluster-scoped (`LegacyManaged`). Credential sources: username+password and bearer-token, both from Secret. (Harbor does not have env/filesystem/federation credentials; Secret-only is correct for this provider.)
- ✅ **7.5** `internal/features/features.go`: `EnableAlphaExternalSecretStores` + `EnableBetaManagementPolicies`.
- ✅ **7.6** Standard flags in `cmd/provider/main.go`: `--debug`, `--sync` (1h), `--poll` (10m), `--poll-state-metric` (5s), `--leader-election`, `--max-reconcile-rate` (default 10), `--enable-management-policies` (true), `--enable-changelogs` (false), webhook/metrics/health addrs.
- **[NICE] ❌ 7.7** — `internal/bootcheck` absent. The inline `canWatchCRD` SAR check achieves equivalent intent (provider degrades gracefully without CRD watch RBAC). Not a functional gap.
- **[NICE] ❌ 7.8** — `ReconciliationPolicy` not embedded in `ProviderConfigSpec`. Low priority for a community provider.
### 8. Examples & E2E Coverage — ⚠️ All MUST+SHOULD pass; 2 NICE gaps
- ✅ **8.1** Both `examples/` and `examples-generated/` exist.
- ✅ **8.2** 21 generated example-ids (20 well-formed; 1 malformed — see below). All resources have a generated example.
- ✅ **8.3** Curated examples carry `meta.upbound.io/example-id` and `testing.upbound.io/example-name`. No `upjet.upbound.io/manual-intervention` annotations (none needed — all resources are automatable).
- ✅ **8.4** Coverage metric output:
generated=21 curated=21 tested(G∩E)=20 gap(G\E)=1 (manual=0 actionable=1) extra(E\G)=1
The phantom gap entry `/v1alpha1/replication` (missing `harbor` group prefix) is a **malformed `example-id` annotation on a dependency `Registry` resource** bundled inside `examples-generated/cluster/harbor/v1alpha1/replication.yaml:23`. The `Replication` resource itself carries the correct id `harbor/v1alpha1/replication` and IS present in curated examples. Root cause: `flattenHarborShortGroup()` removes the `ShortGroup` segment from generated example-ids for `harbor.*` resources when they appear as dependencies. Fix: patch the generator to emit the full group prefix in dependency example-ids. The curated version (`examples/cluster/harbor/v1alpha1/replication.yaml`) already corrects the annotation to `harbor/v1alpha1/replication` (though ideally it should be `harbor/v1alpha1/registry`).
- **[NICE] ⚠️ 8.5** — Content-diff spot-check: `harbor/v1alpha1/project` generated vs curated are byte-identical (`diff` returns no output). Curated examples have not been edited from their generated baseline — values are synthetic defaults. For resources with significant real-world configuration (e.g., retention policies, replication rules), human curation would improve the test signal.
- **[NICE] ❌ 8.6** — Effective actionable gap is 0 (the phantom gap is a generator bug, not a missing example), so near-complete coverage is achieved. However the generator bug should still be fixed.
### 9. Testing & Linting — ✅ All pass
- ✅ **9.1** `internal/clients/harbor_test.go`: table-driven tests for credential resolution (basic auth, bearer token, missing credentials, optional fields). `config/converters/converters_test.go`: converter unit tests. No third-party test packages — stdlib `testing` + `go-cmp`.
- ✅ **9.2** `.golangci.yml` v2 with extensive linter set: `errcheck, govet, gocyclo (≥10), gocritic, goconst, staticcheck, unconvert, unused, misspell, nakedret` plus `gosec, errorlint, exhaustive, bodyclose, contextcheck, durationcheck, nilerr, noctx, rowserrcheck, sqlclosecheck, zerologlint` and more. `revive` is absent but `gocritic` covers its key checks.
- ✅ **9.3** Excludes `zz_\..+\.go$` files and test files for cyclomatic/security linters. `goimports.local-prefixes: github.com/jonasz-lasut/provider-upjet-harbor`.
- ✅ **9.4** `cluster/test/setup.sh`: installs Harbor via Helm with deterministic credentials, seeds both `ProviderConfig` (cluster, `harbor.crossplane.io/v1beta1`) and `ClusterProviderConfig` (namespaced, `harbor.m.crossplane.io/v1beta1`), waits for provider health. `make uptest` wired with `UPTEST_EXAMPLE_LIST`.
- ✅ **9.5** `make crddiff` and `make schema-version-diff` both present in Makefile.
### 10. CI/CD & Governance — ⚠️ All MUST+SHOULD pass; 1 NICE gap
- ✅ **10.1–10.2** `ci.yml`: detect-noop → `report-breaking-changes` (crddiff + schema-version-diff) → lint (golangci-lint-action) → `check-diff` (`make check-diff`, verifies generated code is current) → unit-tests. Generated-code currency verified on every PR.
- ✅ **10.3** `LICENSE` (Apache-2.0), `README.md`, `CODEOWNERS`, `OWNERS.md`.
- ✅ **10.4** `report-breaking-changes` job in `ci.yml:31-56` runs both `make crddiff` and `make schema-version-diff`.
- ✅ **10.5** `publish-provider-package.yml` (reusable workflow via `crossplane-contrib/provider-workflows`). `e2e.yaml` triggers uptest on `/test-examples` PR comment with write/admin permission guard. `tag.yaml` + `backport.yml` present.
- **[NICE] ❌ 10.6** — `stale.yml` absent.
---
## Top Remediation Priorities
1. **(MUST)** `package/crossplane.yaml` — add `meta.crossplane.io/maintainer`, `meta.crossplane.io/source`, `meta.crossplane.io/license`, and `spec.crossplane.version: ">=v2.0.0-0"` (`package/crossplane.yaml`).
2. **(SHOULD)** Promote 20 e2e-tested resources from `v1alpha1` → `v1beta1` across both cluster and namespaced scopes.
3. **(SHOULD)** Fix malformed `example-id: /v1alpha1/replication` on the dependency `Registry` resource in `examples-generated/*/harbor/v1alpha1/replication.yaml:23` — caused by `flattenHarborShortGroup()` dropping the group prefix from generated example-ids.
4. **(SHOULD)** Align publish registry: set `registry_org: crossplane-contrib` (or equivalent) in `publish-provider-package.yml` to match `XPKG_REG_ORGS` in Makefile, and target `xpkg.crossplane.io`.
5. **(SHOULD)** Audit `IdentifierFromProvider` blanket — `harbor_user`, `harbor_label`, `harbor_registry` likely support `NameAsIdentifier` per Terraform import docs.
---
## Untested Resources (actionable G \ E)
None — the phantom `/v1alpha1/replication` entry is a generator bug on a dependency resource, not a missing curated example. All 21 Harbor resources have curated examples. See §8.4 for details. |
jonasz-lasut
force-pushed
the
agentic-skills-bootstrap
branch
from
June 24, 2026 09:22
f32df07 to
32ef513
Compare
jonasz-lasut
marked this pull request as ready for review
June 24, 2026 09:24
Signed-off-by: Jonasz Łasut-Balcerzak <jonasz@upbound.io>
jonasz-lasut
force-pushed
the
agentic-skills-bootstrap
branch
from
June 24, 2026 12:28
32ef513 to
253e30e
Compare
Member
There was a problem hiding this comment.
Thanks, @jonasz-lasut, for these skills. I think this is a very valuable tool for implementing new resources.
I have had a chance to review just the add-upjet-resource skill yet. When I have time, I will also review another skill.
I've mainly pointed out some improvements by giving more details or expanding the sources for resource addition.
Contributor
Author
|
Azure review for a reference: |
Signed-off-by: Jonasz Łasut-Balcerzak <jonasz@upbound.io>
jonasz-lasut
force-pushed
the
agentic-skills-bootstrap
branch
from
June 29, 2026 06:48
d416406 to
f775387
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Adds following skills:
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
review-upjet-previewhas been tested against few providers (harbor, nebius, non-upjet anthropic)add-upjet-resourcehas been tested extensively against nebius provider