docs(plan): внести правки ревью Опуса в план (порядок, ID-миграция, override, array-map-fixed)
This commit is contained in:
Repinoid
@@ -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`.
|
||||
+54
-19
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user