Skip to content

fix(devcontainer): include feature containerEnv in metadata label and warn on parse failure - #231

Merged
skevetter merged 1 commit into
mainfrom
a0ac-5b61-feat-metadata-fixes
May 5, 2026
Merged

fix(devcontainer): include feature containerEnv in metadata label and warn on parse failure#231
skevetter merged 1 commit into
mainfrom
a0ac-5b61-feat-metadata-fixes

Conversation

@skevetter

@skevetterskevetter commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • FeatureConfigToImageMetadata now includes ContainerEnv in the NonComposeBase struct, matching the behavior already present in DevContainerConfigToImageMetadata. Previously feature-level container environment variables were silently dropped from the image metadata label.
  • Downgrades metadata parse failure log level from Errorf to Warnf in both GetImageMetadataFromContainer and GetImageMetadata, since both functions already gracefully degrade by returning an empty config rather than propagating the error.
  • Adds a unit test verifying graceful degradation on corrupt metadata JSON.

Summary by CodeRabbit

Release Notes

  • New Features

    • Container environment variables are now captured and included in metadata for non-Docker Compose configurations.
  • Bug Fixes

    • Image metadata parsing failures now generate warnings instead of errors, improving system resilience when handling malformed metadata.

… warn on parse failure
FeatureConfigToImageMetadata now propagates ContainerEnv to the image
metadata label, matching the behavior of DevContainerConfigToImageMetadata.
Parse failures in GetImageMetadataFromContainer and GetImageMetadata are
downgraded from Error to Warn since the functions already gracefully
degrade by returning an empty config.
@netlify

netlifyBot commented May 5, 2026

Copy link
Copy Markdown

Deploy Preview for devsydev canceled.

NameLink
🔨 Latest commitf69bc78
🔍 Latest deploy loghttps://app.netlify.com/projects/devsydev/deploys/69fa56363d6b2c0008bc42cc

@coderabbitai

coderabbitaiBot commented May 5, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 68f742e1-25b9-4f53-9474-89c0a863f69b

📥 Commits

Reviewing files that changed from the base of the PR and between 00dfefd and f69bc78.

📒 Files selected for processing (2)
  • pkg/devcontainer/metadata/metadata.go
  • pkg/devcontainer/metadata/metadata_test.go

📝 Walkthrough

Walkthrough

The PR adds a ContainerEnv field to the NonComposeBase struct and includes it when building image metadata from features. Error logging for metadata parsing failures is softened from error to warning level. Tests are updated to verify ContainerEnv is now included in metadata.

Changes

ContainerEnv Metadata Capture

Layer / File(s)Summary
Data Shape
pkg/devcontainer/metadata/metadata.go
NonComposeBase struct gains ContainerEnv field and is initialized with the feature's ContainerEnv value in FeatureConfigToImageMetadata.
Error Handling
pkg/devcontainer/metadata/metadata.go
Error logs in GetImageMetadataFromContainer and GetImageMetadata are downgraded to warnings when metadata parsing fails; control flow remains unchanged.
Tests
pkg/devcontainer/metadata/metadata_test.go
TestFeatureConfigToImageMetadata_ExcludesContainerEnv renamed to TestFeatureConfigToImageMetadata_IncludesContainerEnv and now asserts ContainerEnv is populated (FOO=bar, BAZ=qux). New test TestGetImageMetadataFromContainer_WarnsOnParseFailure verifies corrupt metadata labels produce warnings without errors.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • devsy-org/devsy#178: Directly related; both PRs add ContainerEnv to serialized metadata and adjust metadata parsing error logging to warnings.
  • devsy-org/devsy#205: Related through image-based configuration paths; this PR adds ContainerEnv to NonComposeBase while the retrieved PR builds DevContainerConfig from image non-compose base.

Suggested labels

size/m

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title accurately summarizes the two main changes: including feature containerEnv in metadata and downgrading parse failures to warnings.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@skevetter
skevetter marked this pull request as ready for review May 5, 2026 20:44
@skevetter
skevetter enabled auto-merge (squash) May 5, 2026 20:44
@skevetter
skevetter merged commit ad731d4 into mainMay 5, 2026
96 of 98 checks passed
@skevetter
skevetter deleted the a0ac-5b61-feat-metadata-fixes branch May 5, 2026 21:52
skevetter added a commit that referenced this pull request May 5, 2026
… warn on parse failure (#231)
FeatureConfigToImageMetadata now propagates ContainerEnv to the image
metadata label, matching the behavior of DevContainerConfigToImageMetadata.
Parse failures in GetImageMetadataFromContainer and GetImageMetadata are
downgraded from Error to Warn since the functions already gracefully
degrade by returning an empty config.
This was referenced May 16, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@skevetter