Skip to content

feat(migration): add oakIndex migration pattern (Branch F, beta) - #341

Open
bharat941 wants to merge 4 commits into
mainfrom
feat/oak-index-migration-pattern
Open

bharat941 wants to merge 4 commits into
mainfrom
feat/oak-index-migration-pattern

Conversation

@bharat941

Copy link
Copy Markdown
Contributor

Summary

Ports the Oak index migration pattern forward from the stale, fork-only #190 onto current main, moving it to migration/ from the start (per the guava-cache / vault-package-dependencies precedent) and addressing all outstanding review feedback from that PR:

  • Dynamic -custom-N (Mohit): the reference doc now scans the target Cloud Service environment's existing -custom-<n> indexes and computes n+1, instead of assuming the tool's raw -custom-1 output is always correct.
  • Type/datatype compatibility check (Mohit): before presenting the converter's diff as done, verifies jcr:primaryType/oak:isOakIndexDefinition/type and cross-checks JCR value types between legacy and converted definitions, flagging mismatches for manual review instead of silently applying them.
  • Naming convention wording fix (Mohit): corrected to <ootb-name-on-target-cloud-services>-<version>-custom-1.
  • Migration vs. code-assessment placement (rombert): the reference doc now states explicitly why this lives under migration/references/ only — like Dispatcher Conversion (Branch E), it wraps Adobe's aem-cs-source-migration-index-converter tool rather than re-implementing transformation rules.
  • Simplified Index Management (raised by Mohit) is noted as a roadmap item, not implemented here — adopting it as the target format is a separate, larger redesign.

Changes

  • New migration/references/oak-index.md
  • migration/SKILL.md: new "Branch F — Oak Index Migration" section, routing gate, quick-start row, Step 0 strategy table row
  • migration/scripts/bpa-local-parser.js: parses BPA subtypes index.rule.violation / standard.index.modification into oakIndex findings, excluding _COUNT_OID summary rows
  • migration/scripts/unified-collection-reader.js: new processOakIndexFromUnified, wired into the pattern tables and dispatch map
  • migration/scripts/runbook-generator.js: new PATTERN_META.oakIndex entry (bpa-only strategy, matching guavaCache)

Test plan

  • node --test *.test.js in migration/scripts/ — 109/111 pass (2 pre-existing skips), including 2 new oakIndex tests (PATTERN_META shape, dedup + _COUNT_OID exclusion + sub-type split)
  • End-to-end verification against a real customer BPA CSV report: correctly found 84 oak index paths (61 rule violations + 23 standard modifications), matching the independently-confirmed count
  • npx skills-ref validate passes on migration/SKILL.md

Closes discussion in #190.

🤖 Generated with Claude Code

bharat941 and others added 2 commits September 8, 2026 14:41
Ports the Oak index migration pattern forward from the stale, fork-only
PR #190 onto current main, addressing all review feedback:

- Dynamic -custom-N computed from the live CS environment's existing
  indexes instead of assuming -custom-1 (Mohit)
- Type/datatype compatibility check before applying the converter's
  diff (Mohit)
- Corrected naming-convention wording (Mohit)
- Documents why this lives under migration/ rather than code-assessment,
  matching the guava-cache precedent (rombert)

Wires oakIndex end-to-end through bpa-local-parser.js,
unified-collection-reader.js, and runbook-generator.js's PATTERN_META.
Verified against a real customer BPA CSV report (84 oak index paths:
61 rule violations + 23 standard modifications).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Tessl Plugin Lint

⚠️ edge-delivery-services — 2 warning(s)
⚠ Skill 'code-review': SKILL.md is approximately 10352 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'figma-to-content': SKILL.md is approximately 14744 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.

✔ Plugin adobe/aem-edge-delivery-services@0.1.0 is valid
⚠️ app-management — 2 warning(s)
⚠ Skill 'commerce-app-storage': SKILL.md is approximately 5451 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'commerce-app-admin-ui': SKILL.md is approximately 5915 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.

✔ Plugin adobe/commerce-app-management@1.4.0 is valid

app-migration — clean

app-review — clean

run-workflow — clean

⚠️ stardust — 11 warning(s)
⚠ Skill 'stardust': SKILL.md is approximately 5459 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'extract': SKILL.md is approximately 9861 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'direct': SKILL.md is approximately 11694 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'prototype': SKILL.md is approximately 17874 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'migrate': SKILL.md is approximately 6562 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'uplift': SKILL.md is approximately 6515 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'audit': SKILL.md is approximately 5347 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'replica': SKILL.md is approximately 5354 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'reskin': SKILL.md is approximately 5958 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'deploy': SKILL.md is approximately 46367 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.
⚠ Skill 'rollout': SKILL.md is approximately 6978 tokens (recommended maximum: 5000). Consider moving detailed content to separate reference files.

✔ Plugin adobe/stardust@0.21.1 is valid

✅ All 6 plugin(s) lint passed with 15 warning(s) total.

Updated by tessl-lint for commit 010097d.

@rombert rombert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the submission @bharat941 , please see my inline comments.

Comment thread plugins/aem/cloud-service/skills/migration/references/oak-index.md Outdated
Comment thread plugins/aem/cloud-service/skills/migration/references/oak-index.md Outdated
Comment thread plugins/aem/cloud-service/skills/migration/references/oak-index.md Outdated
Comment thread plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.test.js Outdated
@Himanich
Himanich requested a review from tihom88 September 10, 2026 06:40
- use npx -y to resolve the index-converter tool instead of manual
  npm install into a scratch node_modules dir (npx caches it for
  future invocations)
- drop the telemetry section: no such helper exists anywhere in the
  repo, and no other migration reference doc emits telemetry
- drop the aem-guides-wknd-legacy verification section: not part of
  any other reference doc's format
- rename the pattern-count test to avoid churn on future additions
…ion-pattern

# Conflicts:
#	plugins/aem/cloud-service/skills/migration/SKILL.md
@bharat941

Copy link
Copy Markdown
Contributor Author

Addressed all review feedback:

  • npx instead of manual npm install: Step 3 now resolves @adobe/aem-cs-source-migration-index-converter via npx -y (no scratch node_modules, cached under ~/.npm/_npx for future runs). A small working dir is still created, but only because the tool itself does fs.readFileSync(process.cwd() + '/config.yaml') and requires config.yaml in cwd — unrelated to npm vs npx.
  • Telemetry helper: removed — no such helper exists anywhere in the repo, and no other migration reference doc emits telemetry.
  • Verification-on-aem-guides-wknd-legacy section: removed — not part of any other reference doc's format.
  • Test name churn: renamed to 'registry includes all migration patterns with a valid strategy' (dropped the hardcoded count).

Also merged main to resolve the conflict in SKILL.md (kept main's Branch E beta graduation + decluttered starter prompts, plus this PR's Oak Index / Branch F additions; fixed the stale '7-step flow' reference in SKILL.md to '6-step flow' since Step 7 Telemetry was removed from oak-index.md).

Verified: node --test *.test.js → 129/131 pass (2 pre-existing skips), npx skills-ref validate passes.

@rombert

rombert commented Sep 17, 2026

Copy link
Copy Markdown
Member

LGTM, thank you.

@tihom88 - any comments on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants