diff --git a/HISTORY/OPUS/2026-09-22_modifier_plan_review.md b/HISTORY/OPUS/2026-09-22_modifier_plan_review.md new file mode 100644 index 0000000..ec56013 --- /dev/null +++ b/HISTORY/OPUS/2026-09-22_modifier_plan_review.md @@ -0,0 +1,72 @@ +# Opus: ревью плана редизайна модификаторов — 2026-09-22 + +Источник: ответ на `prompt_for_opus_modifier_plan_review.md` (план `PLAN_modifier_redesign.md`). + +## 1. Порядок шагов — скрытые зависимости + +- Шаг 4 (шаблон) ссылается на API из шагов 6–7 → **сначала 5→6→7, потом 4**. +- Шаг 8 (yaml-generator) должен идти ДО регенерации `dev` и до сборки. + +Скорректированный порядок: 1 → 2 → 3 → 5 → 6 → 7 → 4 → 8 → регенерация → 9 → 10. + +## 2. Шаг 5 (вынос JSON-эквивалентности) + +Путь верен. **Оставить реэкспорт-обёртку `JSONStringsEquivalent` в `json_normalize.go`**, +не заменять вызовы по всему resources_core (иначе диф на инстансы, вопрос 7). +Переносятся самодостаточные 5 функций: `JSONStringsEquivalent`, `normalizeJSONIfPossible`, +`encodeCanonicalJSON`, `writeCanonicalJSON`, `normalizeJSONScalarsToStrings`. + +Вариант «готовые строки в core» — отклонить (размазывает нормализацию, не снимает +потребность в JSONStringsEquivalent в core). + +## 3. Шаг 6 — сигнатура и сравнение + +- Маппинг code→param по **двум** алиасам: `p.Code` И `p.SvcOperationCfsParam` (как в + operation_run_bycode.go:50-58). Один `Code` даст пропуски. +- Имя `modifierDesiredEqualsCurrent` — unexported, вызов внутри core. Слово «экспортированный» убрать. +- bool/int/string — `normalizeUniversalValueV6` + сравнение. map-fixed — `JSONStringsEquivalent`. +- **array-map-fixed — дыра:** `normalizeUniversalValueV6` строит дефолт только для + `map-fixed`/`HasPrefix "map"` (params.go:33); `array-map-fixed` туда не попадает → + сравнивать сырые значения через `jsonutil.JSONStringsEquivalent`, не через normalize. +- desired = только явно заданные коды (до досылки live/default), иначе pre-check всегда «равно». + +## 4. Шаг 4.4 Delete=inverse — подводный камень + +- Delete не имеет `plan` (только `req.State`). `reconcile(ctx, plan *Model)` не подходит. + → `reconcile(ctx, model *Model, override map[string]string)`; для inverse override = delete_params. +- `deleteParams` — финальные **wire-строки** (`"false"`, готовый JSON), БЕЗ прогонки через + `ParamFormat`/тип. В реестре `deleteParam{Code, Value}` несёт готовую строку. + +## 5. Шаг 8 — расширение реестра + +Верно. Держать в `serviceSpecificModifiers` (main.go:34), не отдельным реестром. +Структура `modifierException` корректна. При переходе со `map[string]string` на структуру: +`ModifierName` берётся из структуры (сейчас `modName, ok := serviceSpecificModifiers[name]` +— строка 95). + +## 6. Пропущенные кейсы + +- **taint/replace + `delete_strategy=error`** — конфликт: replace = Delete→Create, Delete=error + блокирует → пользователь не сможет заменить error-модификатор. Решить явно: + запретить replace у error (документировать) или отличить «чистый destroy» от replace. +- **unknown в pre-check** — при unknown (computed ref) сравнение невозможно; шаг 6 должен + skip-ить pre-check при unknown (иначе пустая строка даст ложный diff/панику). +- **partial apply** — досылка live для незаданных + pre-check; проверить кейс «часть задана, часть live». + +## 7. Риск сломать инстансы + +Низкий при условиях: +- **НЕ удалять `CompactParams`** (helpers.go:68) — убирается только из modifier-шаблона; + функция нужна инстанс/action. +- **Шаг 7 — новый метод `RunOperationByCodeIdempotent`, НЕ менять сигнатуру** + `RunOperationByCodeWithTimeout`/`RunInstanceOperationUniversalByCode` (зовут инстансы). +- Шаг 1 (поля OperationSpec) аддитивен — безопасно. + +## Дополнительно (не в вопросах) + +- **Шаг 4.3 (ID=identity) — ломающая миграция state.** Смена формата ID изменит ID уже + задеплоенных модификаторов → Terraform форснёт replace. Нужно: либо сохранить старый + формат ID, либо явный state-migration plan. В плане не отмечено. +- **Шаг 3 — `normalizeDeleteStrategy`/`normalizeIdempotency`** — где живут (в loader.go, рядом + с веткой modifier). Не указано. +- **ValidateSpec** — проверка `delete_params.code ∈ op.Params` по lower-code; сверить поле `Code`. diff --git a/PLAN_modifier_redesign.md b/PLAN_modifier_redesign.md index 24ec747..66794d3 100644 --- a/PLAN_modifier_redesign.md +++ b/PLAN_modifier_redesign.md @@ -1,8 +1,11 @@ # ПЛАН реализации: редизайн ресурсов-модификаторов (kind: modifier) Основа: `HISTORY/OPUS/2026-09-22_modifier_architecture_project.md`. +Ревью плана: `HISTORY/OPUS/2026-09-22_modifier_plan_review.md`. Цель — закрыть все классы багов A–E, без костылей, по согласованной архитектуре. -Порядок шагов строгий: контракт → загрузка → шаблон → core → yaml → пересборка. + +Порядок шагов (исправлен по ревью): шаблон (4) зависит от core/resources_core (5–7), +поэтому: 1 → 2 → 3 → 5 → 6 → 7 → 4 → 8 → регенерация → 9 → 10. --- @@ -45,6 +48,9 @@ DeleteParams []Param // из spec.DeleteParams (ConvertParams), толь - `modifier.Idempotency = normalizeIdempotency(op.Idempotency)` (пусто → `none`); - `modifier.DeleteParams = ConvertParams(op.DeleteParams)` (при inverse). +Helpers `normalizeDeleteStrategy`/`normalizeIdempotency` — добавить в `loader.go` +(тот же пакет, рядом с веткой modifier). + Файл: `loader.go`, `ValidateSpec` — расширить fail-fast для modifier: - `delete_strategy` вне enum → ошибка; - `delete_strategy == "inverse"` и пуст `delete_params` → ошибка; @@ -61,25 +67,34 @@ DeleteParams []Param // из spec.DeleteParams (ConvertParams), толь (все заданные поля; решение о досылке — в core). 4.2. **Единый `reconcile()`** — вынести общее тело Create/Update в приватный метод -`reconcile(ctx, plan *Model)`, вызываемый из Create и Update. Устраняет дубль веток. +`reconcile(ctx, model *Model, override map[string]string)`, вызываемый из Create и Update +(override=nil). Устраняет дубль веток. **override нужен для Delete=inverse** (см. 4.4), +так как Delete не имеет plan — только state. 4.3. **ID = identity** — `plan.ID = BuildActionID(instanceUID, modifierName)` (убрать operation из ID). Реализовать через существующий `BuildActionID(instanceUID, "", modifierName)` или новый helper `BuildModifierID(instanceUID, modifierName)`. +⚠️ **миграция state:** смена формата ID изменит ID уже задеплоенных модификаторов → +Terraform форснёт replace. Принять решение ДО: сохранить старый формат ИЛИ явный +state-migration план. По умолчанию — сохранить формат `uid:operation:modifier`, не менять формат. 4.4. **Delete по стратегии**: ``` {{- if eq .DeleteStrategy "error" }} - Delete → AddError (запрет destroy) + Delete → AddError (запрет destroy); ⚠️ конфликт с replace: replace = Delete→Create, + при error пользователь не сможет заменить модификатор. Решение: запретить replace + у error-модификаторов (документировать) или отличить «чистый destroy» от replace. {{- else if eq .DeleteStrategy "inverse" }} - Delete → modify с DeleteParams + досылка live остальных (reconcile-вариант) + Delete → reconcile(state-model, override=delete_params) + (delete_params — финальные wire-строки: "false", готовый JSON; обработать как override) {{- else }} Delete → RemoveResource + AddWarning («эффект остаётся на платформе») {{- end }} ``` 4.5. **Pre-check idempotency** — в reconcile при `eq .Idempotency "check_before_run"`: -передавать флаг в вызов операции (см. шаг 6). +передавать флаг в вызов операции (см. шаг 6). ⚠️ при unknown (computed ref) pre-check +skip — сравнение невозможно. --- @@ -89,8 +104,8 @@ DeleteParams []Param // из spec.DeleteParams (ConvertParams), толь - создать `provider/internal/core/jsonutil/jsonutil.go`: перенести `JSONStringsEquivalent` + `normalizeJSONIfPossible` + `encodeCanonicalJSON` + `writeCanonicalJSON` + `normalizeJSONScalarsToStrings` из `resources_core/json_normalize.go`; -- `resources_core/json_normalize.go` оставить как обёртку (реэкспорт `jsonutil.JSONStringsEquivalent`) - или заменить вызовы на `jsonutil.JSONStringsEquivalent`. +- `resources_core/json_normalize.go` — **оставить реэкспорт-обёртку** `JSONStringsEquivalent` + (не заменять вызовы по resources_core — иначе диф на инстансы). Проверка: `go build ./...`, нет цикла импорта. @@ -100,16 +115,21 @@ DeleteParams []Param // из spec.DeleteParams (ConvertParams), толь Файл: `provider/internal/core/operation_run_bycode.go` (или новый `modifier_compare.go`). -Добавить экспортированный: +Добавить (unexported, вызов внутри core): ```go func (c *UniversalClient) modifierDesiredEqualsCurrent( desired map[string]string, cfsParams []universalCfsParam) bool ``` Логика: -- для каждого desired-кода → найти `universalCfsParam` → live `ParamValue`; -- нормализовать обе стороны `normalizeUniversalValueV6`; -- map-fixed/array-map-fixed → `jsonutil.JSONStringsEquivalent`; -- все поля совпали → true. +- маппинг code→param по **двум** алиасам: `p.Code` И `p.SvcOperationCfsParam` + (как в operation_run_bycode.go:50-58); +- для каждого desired-кода → live `ParamValue`; +- bool/int/string → нормализовать обе стороны `normalizeUniversalValueV6` + сравнение строк; +- map-fixed → `jsonutil.JSONStringsEquivalent`; +- **array-map-fixed → `jsonutil.JSONStringsEquivalent` по сырым значениям, НЕ через normalize** + (`normalizeUniversalValueV6` не строит дефолт для array-map-fixed, params.go:33); +- desired — только явно заданные коды (до досылки live/default); +- если desired содержит unknown (computed ref) — сравнение невозможно, pre-check пропустить. Опционально: добавить в `RunInstanceOperationUniversalByCode` параметр `idempotent bool` (или новый метод-обёртка). В `operation_run_bycode.go` после `fetchOperationCfsParams`: @@ -125,9 +145,9 @@ idle-гейт (`waitForInstanceIdle`) уже стоит выше — не тро ## Шаг 7. Передача флага `idempotent` вплоть до client Цепочка: шаблон → `resources_core.RunOperationByCodeWithTimeout` → `core.RunInstanceOperationUniversalByCode`. -- добавить вариант `RunOperationByCodeIdempotent(...)` в `resources_core/crud.go` - (или расширить сигнатуру существующей, не ломая другие вызовы); -- пробросить флаг в `RunInstanceOperationUniversalByCode`. +- **добавить НОВЫЙ метод `RunOperationByCodeIdempotent(...)` в `resources_core/crud.go`**, + НЕ менять сигнатуру `RunOperationByCodeWithTimeout` (его зовут инстансы); +- пробросить флаг в `RunInstanceOperationUniversalByCode` (новый параметр или обёртка). --- @@ -161,15 +181,18 @@ var serviceSpecificModifiers = map[string]modifierException{ В цикле над ops (там, где `Kind="modifier"`): проставить `op.DeleteStrategy`, `op.Idempotency`, `op.DeleteParams`. +⚠️ При переходе с `map[string]string` на структуру: `ModifierName` берётся из структуры +(сейчас `modName, ok := serviceSpecificModifiers[name]` — строка 95 main.go). + --- ## Порядок коммитов (по смыслу) 1. `feat(lib): delete_strategy/idempotency/delete_params в OperationSpec` -2. `feat(gen): GenModifier расширение + LoadSpecs + ValidateSpec` -3. `refactor(gen): шаблон modifier — reconcile, без CompactParams, ID identity, Delete стратегия` -4. `refactor(core): вынести JSON-эквивалентность в jsonutil` -5. `feat(core): modifierDesiredEqualsCurrent + флаг idempotent` +2. `feat(gen): GenModifier расширение + LoadSpecs + ValidateSpec + normalize-helpers` +3. `refactor(core): вынести JSON-эквивалентность в jsonutil (+реэкспорт)` +4. `feat(core): modifierDesiredEqualsCurrent + RunOperationByCodeIdempotent` +5. `feat(gen): шаблон modifier — reconcile(override), Delete стратегия, ID identity` 6. `feat(yaml): реестр исключений модификаторов (delete_strategy/idempotency)` 7. `test(core,gen): unit-кейсы` 8. `chore(dev): bump версии` @@ -181,3 +204,15 @@ var serviceSpecificModifiers = map[string]modifierException{ Источник-канон — реестр `serviceSpecificModifiers` в `TOOLS/yaml-generator/main.go`. Разметка `delete_strategy`/`idempotency` расширяет этот реестр, а НЕ правится вручную в `generated/dev`. + +--- + +## Решения, нуждающиеся в подтверждении (из ревью) + +1. **ID=identity → миграция state.** Смена формата заставит Terraform replace уже + задеплоенных модификаторов. Предлагаю: НЕ менять формат ID (оставить + `uid:operation:modifier`), а idempotency обеспечить pre-check, не трогая ID. +2. **`error` + replace.** Пользователь не сможет заменить error-модификатор. + Предлагаю: оставить `error` только для «чистого» destroy, документировать запрет replace. +3. **Разметка по default** — `ip_space`: `delete_strategy=error`, `idempotency=check_before_run`; + `network`: `delete_strategy=inverse`, delete_params=[needEnableAVI=false], idempotency=none.