Keep every stored field when editing a K8s endpoint aggregator - #1345
Keep every stored field when editing a K8s endpoint aggregator#1345ikhoon wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe xDS aggregator editor now uses raw YAML for creation and updates. The backend and API support revision-aware compare-and-swap updates, committed revision responses, conflict handling, and revision validation. ChangesxDS aggregator update flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant K8sAggregatorEditor
participant updateK8sAggregator
participant XdsKubernetesService
participant XdsResourceManager
participant Repository
K8sAggregatorEditor->>updateK8sAggregator: Send YAML and loaded revision
updateK8sAggregator->>XdsKubernetesService: Send revision query parameter
XdsKubernetesService->>XdsResourceManager: Apply revision-qualified update
XdsResourceManager->>Repository: Compare and swap target resource
Repository-->>XdsResourceManager: Commit result or conflict
XdsResourceManager-->>updateK8sAggregator: Return YAML and committed revision header
updateK8sAggregator-->>K8sAggregatorEditor: Return saved content and revision
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx (1)
83-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the round-trip contract for
MetadataMappingForm.The fields are only passed through as
watcher.metadataMappingon load/save, so this interface does not validate the persisted schema; if those shapes are a documented Central Dogma xDS convention, cite or move the contract to the backend/proto definition to prevent future UI changes from silently dropping or reformatting unsupported mappings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx` around lines 83 - 117, Clarify the round-trip contract represented by MetadataMappingForm: reference the authoritative Central Dogma xDS backend or proto schema if one exists, or move the mapping type definition there and reuse it in K8sAggregatorEditor. Ensure parseToFormData and buildBody continue preserving all supported metadataMapping fields without silently dropping or reformatting unknown values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 83-117: Clarify the round-trip contract represented by
MetadataMappingForm: reference the authoritative Central Dogma xDS backend or
proto schema if one exists, or move the mapping type definition there and reuse
it in K8sAggregatorEditor. Ensure parseToFormData and buildBody continue
preserving all supported metadataMapping fields without silently dropping or
reformatting unknown values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 061031ec-a7a2-46eb-9590-f40dbc9e5f0f
📒 Files selected for processing (2)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx
33eb336 to
9258fbc
Compare
ikhoon
changed the title
|
You are going to add |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 720-730: Prevent missing revisions from becoming the literal
"undefined": in the useEffect reset flow around parseToFormData, set
loadedRevision only when file.revision is non-null, otherwise leave it
undefined. Apply the same conditional conversion in handleCancel at
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx lines 781-792.
In `@webapp/src/dogma/features/xds/xdsApiSlice.ts`:
- Around line 64-65: Keep FileContentDto.revision numeric for server
deserialization, but convert it to a string at the updateK8sAggregator call site
before constructing the mutation payload. Preserve the existing
String(file.revision) behavior used by the aggregator load/reset paths and
ensure the mutation receives revision as "7"-style text.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 269-281: The default update path in XdsResourceManager must
preserve the successful Command.push revision when calling toYamlResponse,
rather than discarding it; update the success handler to pass that revision
while retaining existing error and redundant-change handling. In
XdsKubernetesService, document that the centraldogma-revision header is present
for committed updates, or ensure redundant 200 responses also supply a usable
revision.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 01e7260b-509d-44db-a5f0-2e1badbe3cb1
📒 Files selected for processing (7)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/src/dogma/features/xds/xdsApiSlice.tswebapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsxwebapp/tests/dogma/features/xds/xdsApiSlice.test.tsxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (2)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx (2)
639-644: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the resource-type and entry-type enums into shared constants.
The allowlists on Lines 639 and 642 duplicate the
<option>values on Lines 678-679 and 695-696. If a new enum value is added to the schema, a developer must update two places. If only the<option>list is updated, the "Ignored fields" block will still report the new value as ignored.♻️ Proposed refactor to share the enum lists
Declare the constants near
MAPPING_MANAGED_KEYS:const RESOURCE_TYPES = ['POD', 'NODE'] as const; const ENTRY_TYPES = ['LABEL', 'ANNOTATION'] as const;Then reference them in both places:
- if (rowOriginal?.resourceType && !['POD', 'NODE'].includes(String(rowOriginal.resourceType))) { + if ( + rowOriginal?.resourceType && + !(RESOURCE_TYPES as readonly string[]).includes(String(rowOriginal.resourceType)) + ) { rowIgnored.resourceType = rowOriginal.resourceType; } - if (rowOriginal?.entryType && !['LABEL', 'ANNOTATION'].includes(String(rowOriginal.entryType))) { + if ( + rowOriginal?.entryType && + !(ENTRY_TYPES as readonly string[]).includes(String(rowOriginal.entryType)) + ) { rowIgnored.entryType = rowOriginal.entryType; }- <option value="POD">POD</option> - <option value="NODE">NODE</option> + {RESOURCE_TYPES.map((t) => ( + <option key={t} value={t}> + {t} + </option> + ))}- <option value="LABEL">LABEL</option> - <option value="ANNOTATION">ANNOTATION</option> + {ENTRY_TYPES.map((t) => ( + <option key={t} value={t}> + {t} + </option> + ))}Also applies to: 678-679, 695-696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx` around lines 639 - 644, Define shared RESOURCE_TYPES and ENTRY_TYPES constants near MAPPING_MANAGED_KEYS, preserving the existing enum values. Replace the inline allowlists in the ignored-fields logic and the corresponding resourceType and entryType option lists with these constants so schema values are maintained in one place.
619-645: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider memoizing the per-row YAML serialization.
rowPreservedandrowIgnoredYamlrunjsYaml.dumpinside the render body of every mapping row.mappingValuescomes fromuseWatch, so the whole list re-renders on each keystroke in any mapping field, and each row re-serializes its preserved and ignored blocks. The watcher-levelpreservedvalue on Line 412 is already memoized, so the two paths are inconsistent.The inputs are
rowOriginalandrowValues?.sourceMode. Extracting the row into a small child component with auseMemoon those two values removes the repeated work and matches the existing pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx` around lines 619 - 645, Extract the per-mapping-row rendering around the rowPreserved and rowIgnoredYaml calculations into a child component, and memoize both jsYaml.dump results with useMemo keyed by rowOriginal and rowValues?.sourceMode. Keep the existing row rendering and preserved/ignored-field behavior unchanged while avoiding serialization on unrelated mapping-field updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 639-644: Define shared RESOURCE_TYPES and ENTRY_TYPES constants
near MAPPING_MANAGED_KEYS, preserving the existing enum values. Replace the
inline allowlists in the ignored-fields logic and the corresponding resourceType
and entryType option lists with these constants so schema values are maintained
in one place.
- Around line 619-645: Extract the per-mapping-row rendering around the
rowPreserved and rowIgnoredYaml calculations into a child component, and memoize
both jsYaml.dump results with useMemo keyed by rowOriginal and
rowValues?.sourceMode. Keep the existing row rendering and
preserved/ignored-field behavior unchanged while avoiding serialization on
unrelated mapping-field updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5648c54d-3cd9-4b2d-ad0b-2d50af4b2955
📒 Files selected for processing (2)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
============================================
- Coverage 69.21% 68.74% -0.48%
+ Complexity 5738 5702 -36
============================================
Files 542 542
Lines 24264 24315 +51
Branches 2802 2809 +7
============================================
- Hits 16795 16716 -79
- Misses 5939 6055 +116
- Partials 1530 1544 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ikhoon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
webapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx (1)
229-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a save response that omits the revision.
Every
mockUpdatestub returns a revision.webapp/tests/dogma/features/xds/xdsApiSlice.test.tsline 73 shows the endpoint can returnrevision: null. In that caseloadedRevisioninK8sAggregatorEditor.tsxkeeps the pre-save value, and the sync effect can restore the stale cached body over the saved content.Add a test that resolves
unwrapwith{ content: 'saved: fresh\n', revision: null }and asserts the editor still showssaved: fresh\nafter the effect re-runs. This test reproduces the issue raised onK8sAggregatorEditor.tsxlines 296-301.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx` around lines 229 - 256, Add a test beside the existing save/cancel coverage that configures mockUpdate to resolve { content: 'saved: fresh\n', revision: null }, performs the edit and save flow, and waits for the synchronization effect to run before asserting the editor still contains the server-saved content. Keep the assertion focused on preserving saved content when the response omits a revision.webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx (1)
256-271: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlso record the synced revision and content marker outside the changed-content branch.
lastSyncedContent.currentandloadedRevisionare updated only whenoriginalContent !== lastSyncedContent.current. If a refetch delivers a newer revision with identical content,loadedRevisionkeeps the older value. The next save then sends a stale base revision.The backend arbitrates unrelated commits, so this does not corrupt data today. Updating
loadedRevisionwhenever the guard passes keeps the local state accurate.♻️ Proposed refactor
if (originalContent !== lastSyncedContent.current) { lastSyncedContent.current = originalContent; setContent(originalContent); setLoadedContent(originalContent); - setLoadedRevision(incomingRevision); } + if (incomingRevision !== '') { + setLoadedRevision(incomingRevision); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx` around lines 256 - 271, Update the synchronization effect around lastSyncedContent and loadedRevision so that, after the file/editing and older-revision guards pass, loadedRevision is recorded for every incoming revision, including when originalContent matches lastSyncedContent.current. Keep content and lastSyncedContent updates conditional on changed content, while ensuring newer identical-content refetches update the base revision used by saving.webapp/tests/dogma/features/xds/xdsApiSlice.test.ts (1)
44-50: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the request body and
Content-Typeas well.This test pins the wire format, but it checks only the method, path, and query parameters. The backend requires
application/yamland the raw YAML body;xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.javalines 490-500 show that contract. A regression that JSON-encodes the body or changes the content type would still pass here.♻️ Proposed additions
expect(url.searchParams.get('summary')).toBe('update & verify'); expect(url.searchParams.get('revision')).toBe('7'); + expect(request.headers.get('Content-Type')).toContain('application/yaml'); + expect(await request.clone().text()).toBe('a: b\n'); expect(result).toMatchObject({ data: { content: 'stored: yaml\n', revision: '12' } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/tests/dogma/features/xds/xdsApiSlice.test.ts` around lines 44 - 50, Extend the request assertions in the xDS API test around the existing fetchSpy request inspection to verify the Content-Type is application/yaml and the body is the raw YAML payload expected by the backend. Keep the current method, URL, query-parameter, and response assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 296-301: Update the save-success handling around setLoadedContent
in K8sAggregatorEditor so the saved response always advances the synced
baseline, including when result.revision is null or otherwise absent. Set the
loaded revision marker from the response without leaving the prior revision
unchanged, while preserving the existing content updates and editing-state flow.
---
Nitpick comments:
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 256-271: Update the synchronization effect around
lastSyncedContent and loadedRevision so that, after the file/editing and
older-revision guards pass, loadedRevision is recorded for every incoming
revision, including when originalContent matches lastSyncedContent.current. Keep
content and lastSyncedContent updates conditional on changed content, while
ensuring newer identical-content refetches update the base revision used by
saving.
In `@webapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx`:
- Around line 229-256: Add a test beside the existing save/cancel coverage that
configures mockUpdate to resolve { content: 'saved: fresh\n', revision: null },
performs the edit and save flow, and waits for the synchronization effect to run
before asserting the editor still contains the server-saved content. Keep the
assertion focused on preserving saved content when the response omits a
revision.
In `@webapp/tests/dogma/features/xds/xdsApiSlice.test.ts`:
- Around line 44-50: Extend the request assertions in the xDS API test around
the existing fetchSpy request inspection to verify the Content-Type is
application/yaml and the body is the raw YAML payload expected by the backend.
Keep the current method, URL, query-parameter, and response assertions
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0a0a0fc-8c93-4d51-878a-6f9bd6bdcf03
📒 Files selected for processing (4)
webapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/src/dogma/features/xds/xdsApiSlice.tswebapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsxwebapp/tests/dogma/features/xds/xdsApiSlice.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/dogma/features/xds/xdsApiSlice.ts
| // Base the next edit on this save even if the invalidated background refetch has not landed yet. | ||
| setContent(result.content); | ||
| setLoadedContent(result.content); | ||
| if (result.revision) { | ||
| setLoadedRevision(result.revision); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A save response without a revision lets the stale cache roll the editor back.
setLoadedRevision runs only when result.revision is truthy. webapp/tests/dogma/features/xds/xdsApiSlice.test.ts line 73 shows revision is null when the centraldogma-revision response header is missing. In that case loadedRevision keeps the pre-save value.
Line 303 then sets editing to false, so the sync effect at lines 257-271 runs again with the not-yet-refetched cache entry. The revision guard at line 262 does not fire, because the cached revision is not lower than the unchanged loadedRevision. The cached pre-save content differs from lastSyncedContent.current, so the effect overwrites content and loadedContent with the pre-save body. The user sees the save reverted, and the next save resends the old body.
Set the synced marker from the save response so the effect treats the saved content as the current baseline.
🐛 Proposed fix
// Base the next edit on this save even if the invalidated background refetch has not landed yet.
+ lastSyncedContent.current = result.content;
setContent(result.content);
setLoadedContent(result.content);
if (result.revision) {
setLoadedRevision(result.revision);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Base the next edit on this save even if the invalidated background refetch has not landed yet. | |
| setContent(result.content); | |
| setLoadedContent(result.content); | |
| if (result.revision) { | |
| setLoadedRevision(result.revision); | |
| } | |
| // Base the next edit on this save even if the invalidated background refetch has not landed yet. | |
| lastSyncedContent.current = result.content; | |
| setContent(result.content); | |
| setLoadedContent(result.content); | |
| if (result.revision) { | |
| setLoadedRevision(result.revision); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx` around lines 296 -
301, Update the save-success handling around setLoadedContent in
K8sAggregatorEditor so the saved response always advances the synced baseline,
including when result.revision is null or otherwise absent. Set the loaded
revision marker from the response without leaving the prior revision unchanged,
while preserving the existing content updates and editing-state flow.
Motivation: Two aggregator edits that overlap in time both succeeded: the second one was built from a document read before the first one landed, so it rolled that change back with no sign anything had happened. Separately, the load-balancing policy on an aggregator was stored but never reached the endpoints generated from it, so setting it did nothing. Modifications: - The update endpoint takes the revision the client read the aggregator at and uses it as the base revision of the commit, so Central Dogma rejects the push itself: a conflict is 409, an unknown revision 400. Central Dogma compares revisions per repository, so any commit in the group makes an open editor stale; omitting the parameter keeps the previous always-apply behaviour. Mapping ChangeConflictException to 409 is shared by every xDS resource type, which previously answered 500. - The policy is copied into the generated ClusterLoadAssignment and into the preview, and is now validated on create and update: Envoy applies at most one drop overload and rejects the whole assignment when it sees more, or an overprovisioning factor of zero. Aggregators that already store such a policy were harmless while it was ignored and would now be served, so they are worth checking before this ships. Result: An aggregator edit based on a stale read is refused instead of silently overwriting a newer one, and a policy set on an aggregator takes effect.
Motivation: The aggregator form modelled part of the schema and rebuilt the document from that model on save, so every field it had no editor for was erased: distinctEndpoint, the metadata mappings, and the policy. An operator who edited an aggregator through the console discarded them without being told; losing distinctEndpoint, for instance, brings duplicate endpoints back on the next rolling restart. Modifications: - The form holds the stored document itself, so a field it renders is the field that gets saved and a field it does not render cannot be silently rebuilt away. What is left is pruning empty values before the document is written, and two adapters the document shape cannot express as form fields: the additional-properties map, whose keys are data, and the source-key oneof. - Add the missing editors: distinct endpoint, metadata mappings, and the policy. Drop overloads stay read-only because Envoy applies at most one and rejects the endpoints outright when it sees more; a stored one is shown and saved back unchanged. The two policy fields the Armeria xDS client does not read are marked as Envoy-only. - Send the revision the form was loaded at, and surface the server's 409 as a prompt to reload. Result: Editing an aggregator through the console keeps every field it was stored with, and a save based on a stale read is refused instead of rolling back a concurrent change.
ab120e9 to
a5a7f83
Compare
ikhoon
changed the title
Right. In addition, I refactored two things:
|
Motivation: `next dev` and `npm run build` both write to `build/web`, and `:webapp:runTestServer` runs the latter. Starting a test server while a dev server is up therefore deletes the dev server's manifests, and the page turns into "missing required error components, refreshing...", a 500, or silently stops hot-reloading — with nothing to suggest the backend did it. Modifications: - `distDir` reads NEXT_DIST_DIR when set, so the dev server can be pointed elsewhere. The default is unchanged, so the production build and CI are unaffected. Result: `NEXT_DIST_DIR=.next npm run develop` survives a gradle build running beside it.
Motivation: The form named the schema's fields and left it at that. An operator who had not written this YAML before could not tell what a watcher was, which direction "Trust certificates" was safe in, that priority 0 is the highest, or what a metadata mapping copies from where. The schema was on screen; its meaning was not. Modifications: - Every field carries a one-line hint, and each group carries a caption: what reaching the cluster needs, what ends up in the endpoints, what a locality is reported for, what a mapping copies. - Split each source into "Cluster access" and "Endpoints", so how to reach Kubernetes is separate from what is read out of it, and name the card after what it is: a Kubernetes endpoint source. - Give the page a type scale rather than three weights of the same size: sections at 12px uppercase on a rule, labels at 14px semibold, hints at 12px. Policy is one field per row, and mapping rows pair the six fields two by two, so nothing wraps or sits alone. - A read-only select no longer dims the value it is showing, which made a chosen value look unset. Result: The form reads as an explanation of what it is about to save, not as a list of proto field names.
|
|
Empty form: |
Motivation: The form put everything on screen at once and left the reader to work out what belonged together. The policy, which most aggregators never set, took as much room as the fields that matter; the button that adds a source read as part of the card above it; and adding one left the cursor wherever it happened to be rather than in the new card. Modifications: - Collapse the policy behind its own header, with a line saying what it is for, and open it whenever one is stored. In read mode an aggregator without a policy no longer shows an empty section. - Give the sources their own colour, shared by the card heading and the button that adds one, so the two read as the same thing. The button sits between the cards and the policy, out of both. - Adding a source moves the cursor to the new card's first field. - Drop the hint under the aggregator ID, which said what the label already says. Result: The form opens on the fields an operator actually fills in, and adding a second source continues where the typing left off.
3 participants
Motivation:
The K8s endpoint aggregator form modelled only part of the schema and rebuilt the document from that
model on save, so distinctEndpoint, the metadata mappings and the policy were erased. Losing
distinctEndpoint brings duplicate endpoints back on the next rolling restart.
Two more problems sat next to it: overlapping edits both succeeded, the later one rolling the earlier
back unnoticed, and a stored policy never reached the endpoints generated from it.
Modifications:
The form holds the stored document itself, so a field it does not render can no longer be rebuilt
away. Distinct endpoint, metadata mappings and the policy are now editable; drop overloads are
shown read-only, because Envoy applies at most one and rejects the endpoints when it sees more.
The update endpoint takes the revision the client read the aggregator at and commits on top of it:
a conflict is 409, an unknown revision 400. Revisions are compared per repository, so any commit in
the group makes an open editor stale. Omitting the parameter keeps the previous behaviour.
The policy reaches the generated endpoints and the preview, and is validated on write.
Result:
Editing an aggregator through the console keeps every field it was stored with, a save based on a
stale read is refused, and a policy set on an aggregator takes effect.
An aggregator that already stores a policy Envoy would reject was harmless while it was ignored, and
will now be served. Worth checking before this ships.