[kelvinprabhu]

Closes #127
Refs OpenAgriNet/helmcharts#123 (review comment)

Changes

Change	Reason
config/mappings/ deleted (11 files, 6 providers).	helmcharts quick-start/config/mappings/ is the copy deployments read; two copies drifted (knowledge lacked #105).
make sync-mappings: shallow sparse checkout of helmcharts at HELMCHARTS_REF (pinned SHA) into gitignored .helmcharts/, once per ref. test, cover, test-ci depend on it; ci.yml runs it as its own step.	One fetch per ref, offline after; a helmcharts push can't change this repo's CI without a ref bump.
Mapping tests serve .helmcharts/... over a local test server; fail with "run make sync-mappings" if missing.	The tested mapping is the shipped one.
docs/adr/0005-mappings-in-helmcharts.md.	Records the decision.

Note

• A mapping change lands in helmcharts first, then a commit here bumps HELMCHARTS_REF.
• HELMCHARTS_REF must be a full commit SHA.

[github-actions]

🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)

View full run

Go dependencies

No findings at CRITICAL,HIGH,MEDIUM,LOW.

Container image

No findings at CRITICAL,HIGH,MEDIUM,LOW.

[github-actions]

📊 Test Coverage: ➖ Not applicable — no non-test Go files changed vs origin/development, so there are no lines to measure and no percentage to report.

[kelvinprabhu]

Need some changes

[kelvinprabhu]

can we have one mapping url config file where for all we will add the url so for test file we dont have to add seperate and next addiction be in one
single place

[kelvinprabhu]

reverting the decision and keep the mappings on there seperate files so we can have different URL as needed

[kelvinprabhu]

self reviewed Done |. need suggestion on this approch | pkg/plugin/implementation/internal/mappingurls/mappingurls.go
@manjudr

[kelvinprabhu]

Open Items : knowledge mapping mismatch.

@manjudr

The two copies of knowledge/knowledge-advisory.select.yaml were not the same. helmcharts 0c7d5c9b (#105) added provider_id to the search body, taken from commitments[0].offer.provider.id and left out when no provider is named. network-adapter never got that change.

So deployments ran the helmcharts version while the tests here checked the old one. With this PR the tests read helmcharts, so the drift is gone. But no KnowledgeAdvisory test checks provider_id, which is why the mismatch went unnoticed.

[manjudr]

The direction is right. The mappings should live in helmcharts only, and the deletion is clean: nothing else in the repo references config/mappings/ (no Dockerfile, no workflow, no runtime code), all 11 files exist on helmcharts development, and 10 of the 11 are byte-identical. The mechanism chosen to replace them is where the problems are.

Findings below were checked against the branch: suites run on both base and head, all 11 deleted files diffed against helmcharts, and an offline run simulated.

---

1. The unit tests now need the public internet to pass

Problem

pkg/plugin/implementation/WeatherObservation/mappings_test.go:30 and the five sibling constants point the mapper at raw.githubusercontent.com. These are not live tests — they are the ordinary suite make test-ci runs.

Counting the traffic with a local CONNECT proxy: one go test run of the five changed packages opens 219 TLS connections to raw.githubusercontent.com. With the network removed, the test does not skip, it fails:

mappings_test.go:144: Run() returned an unexpected error: jsonmapper: failed to fetch
mapping "https://raw.githubusercontent.com/OpenAgriNet/helmcharts/development/...": ...

Runtime on the five packages goes from 10.4s to 40.9s. Grievance alone goes 2.4s to 14.7s.

This contradicts the convention the repo already states in pkg/plugin/implementation/catalogcrawler/publishsweep_live_test.go:20: "Skipped unless MANDI_LIVE=1, so go test ./... stays hermetic."

Recommendation

Fetch the mappings once, outside the test. Add a make sync-mappings target that checks out helmcharts into a gitignored directory, call it from ci.yml before make test-ci, and have the tests read that directory from disk — the same serveMappings helper, pointed at the synced path instead of config/mappings/. One fetch per run, offline-capable, and the mapping is still maintained in exactly one place.

---

2. The URL is pinned to a moving branch

Problem

All six constants pin .../helmcharts/development/.... ci.yml:40 runs the suite on release-* branches, so a release branch validates itself against whatever helmcharts development holds that day — including mappings that have not been released. In the other direction, any commit to helmcharts development can turn network-adapter CI red on a PR that touched nothing related.

Recommendation

Pin a commit SHA or a helmcharts tag, in one place, and bump it as a deliberate commit. If the sync-step approach in finding 1 is taken, this is the HELMCHARTS_REF ?= <sha> variable in the Makefile.

---

3. The same URL is copy-pasted into six files

Problem

WeatherObservation/mappings_test.go:30, MandiPrice/mappings_test.go:31, KnowledgeAdvisory/mappings_test.go:29, AgricultureFacility/mappings_test.go:37, Grievance/mappings_test.go:29, Grievance/pmkisan_mappings_test.go:38.

Every one of them is the identical host, org, branch and path prefix; only the last segment (mausamgram, agmarknet, ...) differs. Repointing the branch is six edits across five packages.

This was right the first time with pkg/plugin/implementation/internal/mappingurls/mappingurls.go, and was reverted as "keep the mappings on their separate files so we can have different URL as needed." Nothing in the diff needs a different URL — there is one base and six directory names.

Recommendation

Restore internal/mappingurls with a single base constant and a For(provider string) string helper. Each test file becomes var mappingsURL = mappingurls.For("mausamgram"). A per-provider override can be added the day one is actually needed.

---

4. The one drift this PR uncovered is still not covered by a test

Problem

The knowledge-advisory.select.yaml copies had genuinely diverged — confirmed. helmcharts adds provider_id from commitments[0].offer.provider.id, omitted when no provider is named; network-adapter's copy had neither.

The premise of the PR is that the tested mapping is the shipped one. But as flagged in the open-items comment, no test in KnowledgeAdvisory/mappings_test.go asserts provider_id. So after this merges, the suite fetches the helmcharts file and still cannot tell whether that field works. The same class of drift — a field added in helmcharts with no assertion here — passes silently again.

Recommendation

Add two assertions to KnowledgeAdvisory/mappings_test.go in this PR: a request carrying commitments[0].offer.provider.id sends provider_id in the search body, and a request without it sends no provider_id key at all. It is the proof that the premise of the change holds.

---

5. The cross-repo decision is recorded as a repeated code comment, not an ADR

Problem

// mappingsURL is where the shipped mappings live: helmcharts development, which releases are cut from. appears five times, at WeatherObservation/mappings_test.go:29, MandiPrice/mappings_test.go:30, KnowledgeAdvisory/mappings_test.go:28, AgricultureFacility/mappings_test.go:34 and the Grievance pair.

"This repo's tests take a build-time dependency on another repo" is an architectural decision with a cost and a rejected alternative. Five copies of a one-line comment is the wrong place for it, and docs/adr/ already exists for the right one.

Recommendation

Write docs/adr/0005-provider-mappings-live-in-helmcharts.md covering why the copies were dropped, why helmcharts is the source, and what is pinned. Delete all five comments and link the ADR once, from the internal/mappingurls package doc.

---

Resolved

• Self review — done, on two commits. Gate passed.
• Unwanted changes — none. The diff is 11 mapping deletions, the test rewiring, one README and one YAML comment. No drive-by edits.
• Build health — go vet clean on all changed packages, no orphaned imports left behind, gofmt clean on every file this PR touches.
• Deletion safety — confirmed nothing else in the repo reads config/mappings/, and all 11 files are present on helmcharts development.

[kelvinprabhu]

The direction is right. The mappings should live in helmcharts only, and the deletion is clean: nothing else in the repo references config/mappings/ (no Dockerfile, no workflow, no runtime code), all 11 files exist on helmcharts development, and 10 of the 11 are byte-identical. The mechanism chosen to replace them is where the problems are.

Findings below were checked against the branch: suites run on both base and head, all 11 deleted files diffed against helmcharts, and an offline run simulated.

1. The unit tests now need the public internet to pass

Problem

pkg/plugin/implementation/WeatherObservation/mappings_test.go:30 and the five sibling constants point the mapper at raw.githubusercontent.com. These are not live tests — they are the ordinary suite make test-ci runs.

Counting the traffic with a local CONNECT proxy: one go test run of the five changed packages opens 219 TLS connections to raw.githubusercontent.com. With the network removed, the test does not skip, it fails:

mappings_test.go:144: Run() returned an unexpected error: jsonmapper: failed to fetch
mapping "https://raw.githubusercontent.com/OpenAgriNet/helmcharts/development/...": ...

Runtime on the five packages goes from 10.4s to 40.9s. Grievance alone goes 2.4s to 14.7s.

This contradicts the convention the repo already states in pkg/plugin/implementation/catalogcrawler/publishsweep_live_test.go:20: "Skipped unless MANDI_LIVE=1, so go test ./... stays hermetic."

Recommendation

Fetch the mappings once, outside the test. Add a make sync-mappings target that checks out helmcharts into a gitignored directory, call it from ci.yml before make test-ci, and have the tests read that directory from disk — the same serveMappings helper, pointed at the synced path instead of config/mappings/. One fetch per run, offline-capable, and the mapping is still maintained in exactly one place.

2. The URL is pinned to a moving branch

Problem

All six constants pin .../helmcharts/development/.... ci.yml:40 runs the suite on release-* branches, so a release branch validates itself against whatever helmcharts development holds that day — including mappings that have not been released. In the other direction, any commit to helmcharts development can turn network-adapter CI red on a PR that touched nothing related.

Recommendation

Pin a commit SHA or a helmcharts tag, in one place, and bump it as a deliberate commit. If the sync-step approach in finding 1 is taken, this is the HELMCHARTS_REF ?= <sha> variable in the Makefile.

3. The same URL is copy-pasted into six files

Problem

WeatherObservation/mappings_test.go:30, MandiPrice/mappings_test.go:31, KnowledgeAdvisory/mappings_test.go:29, AgricultureFacility/mappings_test.go:37, Grievance/mappings_test.go:29, Grievance/pmkisan_mappings_test.go:38.

Every one of them is the identical host, org, branch and path prefix; only the last segment (mausamgram, agmarknet, ...) differs. Repointing the branch is six edits across five packages.

This was right the first time with pkg/plugin/implementation/internal/mappingurls/mappingurls.go, and was reverted as "keep the mappings on their separate files so we can have different URL as needed." Nothing in the diff needs a different URL — there is one base and six directory names.

Recommendation

Restore internal/mappingurls with a single base constant and a For(provider string) string helper. Each test file becomes var mappingsURL = mappingurls.For("mausamgram"). A per-provider override can be added the day one is actually needed.

4. The one drift this PR uncovered is still not covered by a test

Problem

The knowledge-advisory.select.yaml copies had genuinely diverged — confirmed. helmcharts adds provider_id from commitments[0].offer.provider.id, omitted when no provider is named; network-adapter's copy had neither.

The premise of the PR is that the tested mapping is the shipped one. But as flagged in the open-items comment, no test in KnowledgeAdvisory/mappings_test.go asserts provider_id. So after this merges, the suite fetches the helmcharts file and still cannot tell whether that field works. The same class of drift — a field added in helmcharts with no assertion here — passes silently again.

Recommendation

Add two assertions to KnowledgeAdvisory/mappings_test.go in this PR: a request carrying commitments[0].offer.provider.id sends provider_id in the search body, and a request without it sends no provider_id key at all. It is the proof that the premise of the change holds.

5. The cross-repo decision is recorded as a repeated code comment, not an ADR

Problem

// mappingsURL is where the shipped mappings live: helmcharts development, which releases are cut from. appears five times, at WeatherObservation/mappings_test.go:29, MandiPrice/mappings_test.go:30, KnowledgeAdvisory/mappings_test.go:28, AgricultureFacility/mappings_test.go:34 and the Grievance pair.

"This repo's tests take a build-time dependency on another repo" is an architectural decision with a cost and a rejected alternative. Five copies of a one-line comment is the wrong place for it, and docs/adr/ already exists for the right one.

Recommendation

Write docs/adr/0005-provider-mappings-live-in-helmcharts.md covering why the copies were dropped, why helmcharts is the source, and what is pinned. Delete all five comments and link the ADR once, from the internal/mappingurls package doc.

Resolved

• Self review — done, on two commits. Gate passed.
• Unwanted changes — none. The diff is 11 mapping deletions, the test rewiring, one README and one YAML comment. No drive-by edits.
• Build health — go vet clean on all changed packages, no orphaned imports left behind, gofmt clean on every file this PR touches.
• Deletion safety — confirmed nothing else in the repo reads config/mappings/, and all 11 files are present on helmcharts development.

Re review 5854705058, addressed in 81ef7b2 and 459608f:

1. Network in unit tests: fixed. make sync-mappings does one shallow sparse checkout of helmcharts into gitignored .helmcharts/, once per ref; test, cover, test-ci depend on it and ci.yml runs it as its own step. Tests serve the files from disk over a local test server, so no fetches during the run (five packages ~3s uncached). Without a sync, tests fail with ... is missing: run make sync-mappings.
2. Moving branch: fixed. HELMCHARTS_REF in the Makefile pins a full SHA (b1861de7, helmcharts development at the feat: cache registry data in the adapter (#109) #123 merge); a branch name is refused. A mapping change is taken by a commit that bumps it.
3. One URL helper: kept per file on purpose. A plugin's mappings may need to come from somewhere other than helmcharts: another URL, or a different branch for one plugin. With a constant in each file, one plugin can be pointed elsewhere without touching the others; a shared helper would force one base on all of them.
4. provider_id test: no code change. Checked locally: the helmcharts mapping sends provider_id when the offer names a provider and leaves the key out when it doesn't; the KnowledgeAdvisory suite passes against it.
5. ADR: added docs/adr/0005-mappings-in-helmcharts.md; the repeated comments are gone.
   @manjudr