fix: manual building block outputs computed from API; BBD apply regression coverage#197
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds developer tooling to dump acceptance-test Terraform into git-ignored scratch/ dirs and run them with a dev-built provider; implements server- and provider-side derivation and validation of outputs for manual building blocks, updates schema/docs/examples/changelog, and adds tests for immutability and manual-output validation. ChangesScratch-config-testing developer infrastructure
Manual building-block outputs behavior and validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
📊 Test Coverage: 73.5%Top uncovered filesCoverage measured across all packages with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.agents/skills/scratch-config-testing/SKILL.md:
- Around line 19-21: The fenced code block containing "set -a && source .env &&
set +a # MESHSTACK_ENDPOINT, MESHSTACK_API_KEY, MESHSTACK_API_SECRET" needs
surrounding blank lines and the code-fence should include a language identifier
(e.g., text) to satisfy markdownlint MD031/MD040; update that block by adding an
empty line before and after the triple-backtick fence and change ``` to ```text,
and do the same for the other fenced example starting at the fence around lines
56-59 so both examples have blank lines around them and explicit language
identifiers.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02934715-ae30-4eba-982b-606bffbec0b6
📒 Files selected for processing (5)
.agents/skills/scratch-config-testing/SKILL.md.gitignoreAGENTS.mdinternal/provider/building_block_definition_resource_test.gointernal/provider/provider_test.go
grubmeshi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/provider/building_block_definition_resource.go`:
- Around line 176-177: The loop over outputAssignmentTypes currently permits
empty assignmentType values, allowing manual outputs with omitted
assignment_type to bypass validation; update the logic in outputAssignmentTypes
and the caller loop that iterates "for key, assignmentType := range
outputAssignmentTypes(outputs)" to treat empty string the same as NONE and
reject it: if assignmentType == "" || assignmentType ==
client.MeshBuildingBlockDefinitionOutputAssignmentTypeNone.String() then return
an error (or mark invalid) for that output key. Also adjust the code path that
currently drops null/omitted assignment values (the branch that removes entries
when assignment is nil) so it no longer silently removes them but instead
surfaces/returns the same validation error for omitted (nil/empty)
assignment_type in outputs.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f211f379-ef02-4857-b019-3185b1c10d80
📒 Files selected for processing (9)
CHANGELOG.mddocs/resources/building_block_definition.mdexamples/resources/meshstack_building_block_definition/resource_03_manual.tfexamples/resources/meshstack_building_block_v2/test-support_01_workspace.tfexamples/resources/meshstack_building_block_v2/test-support_02_tenant.tfinternal/clientmock/mock_buildingblock_definition_version.gointernal/provider/building_block_definition_resource.gointernal/provider/building_block_definition_resource_schema.gointernal/provider/building_block_definition_resource_test.go
grubmeshi
force-pushed
the
fix-bbd-apply
branch
6 times, most recently
from
f37ea67 to
ab38fd0
Compare
grubmeshi
force-pushed
the
fix-bbd-apply
branch
2 times, most recently
from
8faec3a to
d70c796
Compare
This was
linked to
issues
Jun 10, 2026
Closed
Turn an acceptance test's generated HCL (or hand-written HCL) into a
self-contained, re-runnable Terraform config under git-ignored scratch/,
run against a local meshStack via the dev-built provider binary.
- ApplyAndTest dumps each step's config to scratch/<test>/stepNN/
{main.tf,provider.tf} when MESHSTACK_SCRATCH_DUMP is set, then returns
without running the test (dump-only; no backend or terraform binary needed).
provider.tf resolves the dev-built provider via dev_overrides, so the dump
is runnable standalone — unlike the framework's TF_ACC_PERSIST_WORKING_DIR.
- Add the scratch-config-testing skill: build, ~/.terraformrc.dev dev_override
setup, dumping/hand-writing a config, and running plan/apply against a local
meshStack.
- Ignore scratch/ and wire the skill into AGENTS.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a Terraform-implementation subtest that creates a building block definition draft, releases it, then flips it back to draft together with a pre_run_script change. It asserts the already-released version stays immutable: its content_hash is unchanged across the new draft creation and no "inconsistent result after apply" occurs. This is the provider-side reproducer for #131. The underlying fix is in the backend (meshcloud/meshfed-release#10116), which previously shared the implementation object across versions. Closes #131 via internal PR meshcloud/meshfed-release#10116 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Manual building block definitions have their outputs derived from the inputs on the backend (one output per input, assignment type NONE, with SINGLE_SELECT/MULTI_SELECT/LIST translated). Configuring outputs that mirror the inputs caused "Provider produced inconsistent result after apply" whenever the backend added or changed an output the config did not predict, especially when toggling draft false->true together with input changes (#131, #176). version_spec.outputs is now Optional+Computed without a static default: omit it for manual building blocks and it is reconciled from the API. The plan leaves outputs (and the content hash) unknown when the inputs or the configured PLATFORM_TENANT_ID outputs change, and reuses the reconciled state value otherwise. ValidateConfig rejects manual outputs with any assignment_type other than PLATFORM_TENANT_ID, which the backend still honors to mark the tenant-id output. The mock client mirrors the backend's manual output generation so unit tests cover the behavior. Closes #176 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rotating a sensitive input's secret on a released (non-draft, immutable) building block definition version failed with an opaque "Failed to determine content hash ... [plaintext]" error: the planned version_spec DTO carries the rotated secret's plaintext, which the content hash disallows (the backend only ever returns secret hashes). Released versions are immutable, so the rotation must be rejected. Detect the plaintext secret in the DTO (an unambiguous version_spec change) and return the existing "Updating a version_spec in non-draft state is not allowed" error with guidance to create a new draft version first, instead of feeding plaintext into the content hash. Adds a regression test covering create -> release -> rotate. Closes #196 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Companion to the released-version immutability fix (#131): CI implementations like github_workflows carry an integration_ref besides the implementation. Add a regression test that releases a github BBD version, then re-drafts it (draft false->true) while switching to a different integration, asserting the new draft v2 points at the new integration while the released v1 keeps its original integration and stays immutable (stable content_hash). Adds a BBDGithubTwoIntegrations testconfig builder that wires the BBD to a first integration plus a second github integration (reusing the integration test-support file via RenameKey) so the test can switch between them. The BBD depends_on the first integration so teardown deletes the BBD before both integrations (the released version pins the first on the backend, which refuses integration deletion while still referenced). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📋 This PR has been linked to Feature Shipping Tracker #1344: Fix Manual Building Block Output Type Handling (SINGLE_SELECT / MULTI_SELECT). DoD item 5 (Terraform provider updated) has been checked off. Remaining open items: docs (meshcloud/meshfed-release#9999), API docs (meshcloud/meshfed-release#10123), hub reference examples (meshcloud/meshstack-hub#198), and Canny Known Issue resolution. [edited: corrected cross-repo PR references to use |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the
meshstack_building_block_definition"Provider produced inconsistent result after apply" failures from #131 and #176.1. Manual outputs computed from the API (#176, manual side of #131)
The backend derives a manual building block's outputs from its inputs (one output per input,
assignment_type = NONE, withSINGLE_SELECT/MULTI_SELECT/LISTtranslated). Mirroring those outputs in config — or letting the old static{}default fight the API — produced inconsistent-result errors whenever the backend added/changed an output the plan didn't predict, especially when togglingdraftfalse→true together with input changes.Changes:
version_spec.outputsis nowOptional+Computedwith no static default (+UseStateForUnknown). Omit it for manual building blocks; it is reconciled from the API response.ModifyPlanleaves manualoutputs(and the dependentcontent_hash) unknown when the inputs or the configuredPLATFORM_TENANT_IDoutputs change, and reuses the reconciled state value otherwise (no perpetual diff).ValidateConfigrejects manualoutputswith anyassignment_typeother thanPLATFORM_TENANT_ID(the only assignment the backend preserves — used to mark the tenant-id output). ManualCreate/Updatesource that hint from config.outputsfor manual building blocks; docs regenerated. Breaking: existing manual configs must remove non-PLATFORM_TENANT_IDoutputsblocks (noted in CHANGELOG v0.21.1).New tests:
TestAccBuildingBlockDefinition/07_manual_computed_outputs(create → release → re-draft with an added input, asserting reconciled outputs incl.SINGLE_SELECT→STRINGand a stable released-version hash) andTestAccBuildingBlockDefinitionManualOutputsValidation.2. Released-version mutation regression test (terraform side of #131)
TestAccBuildingBlockDefinition/06_release_redraft_implementation_changereleases a terraform version, then flips it back to draft with apre_run_scriptchange, asserting the already-released version stays immutable. The backend fix is meshcloud/meshfed-release#10116.Verification
task lintclean; full unit suite passes.TestAccBuildingBlockDefinition(all subtests),TestAccBuildingBlockV2,TestAccLandingZone, and the BBD data sources.🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
version_spec.outputsmust be omitted; outputs are now derived from inputs by the backend.PLATFORM_TENANT_IDare no longer accepted for manual blocks.Bug Fixes
New Features
Documentation