feat: default to manifest.json over get-manifest hook - #629
Conversation
srtaalej
added
enhancement
srtaalej
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #629 +/- ##
==========================================
- Coverage 72.22% 72.20% -0.02%
==========================================
Files 238 238
Lines 20072 20093 +21
==========================================
+ Hits 14497 14509 +12
- Misses 4302 4308 +6
- Partials 1273 1276 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Super appreciate the changes toward this 🌟
I left a comment on precedence of manifestations and am thinking that the fallback manifest.json file allows us to consider the manifest "CLI-managed" while with a hook ought mean not.
This pattern has a slow rollout with hooks needing to be updated too, but I'm most optimistic for the changes it'll unlock with confidence in changing a project manifest.
| if exists, _ := afero.Exists(c.fs, manifestPath); exists { | ||
| return c.readManifestFile(manifestPath) | ||
| } | ||
| return c.getManifestFromHook(ctx, sdkConfig, hookExecutor) |
There was a problem hiding this comment.
🔭 thought: We should reverse this order to check if a get-manifest hook exists and fallback to reading "manifest.json" file. Deprecating the hook might happen at the hook package while the CLI continues to support it.
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
🔍 Before testing more I want to cut scope on this change. I think other PRs can follow to ensure we're not clobbering an SDK managed manifest!
| if !exists { | ||
| return WriteBackResult{ | ||
| Warning: fmt.Sprintf("No %s found in project root — merged manifest was not written locally", manifestFileName), | ||
| }, nil | ||
| fresh, err := marshalFresh(manifest) | ||
| if err != nil { | ||
| return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err) | ||
| } | ||
| if err := atomicWriteFile(fs, manifestPath, fresh, 0644); err != nil { | ||
| return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err) | ||
| } | ||
| return WriteBackResult{Written: true, FilePath: manifestPath}, nil | ||
| } |
There was a problem hiding this comment.
🦠 suggestion: Let's keep this change for a separate PR!
| return c.readManifestFile(manifestPath) | ||
| } | ||
|
|
||
| func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) { |
There was a problem hiding this comment.
| func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) { | |
| func (c *ManifestClient) getManifestFromFile(sdkConfig hooks.SDKCLIConfig) (types.SlackYaml, error) { |
🌵 thought: I think we should match the function name convention here and also keep path specific logic contained within this function. I'm less confident of the second thought FWIW!
2 participants
Summary
GetManifestLocalnow prefers readingmanifest.jsondirectly from the project root, falling back to theget-manifesthook only when the file doesn't existreadManifestFiletogetManifestFromFileto match thegetManifestFromHooknaming conventionCloses #627
Context
Projects relying on a
get-manifesthook instead of a staticmanifest.jsonhit issues when manifest sync tried to write back results but nomanifest.jsonexisted.This makes
manifest.jsonthe preferred source of truth for local manifests while maintaining backward compatibility — the hook is still invoked for projects that don't have amanifest.json.Test plan
make testpassesmanifest.jsonreads file directly (hook not invoked)manifest.jsonfalls back to get-manifest hook