Repository navigation
Conversation
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
✅ Deploy Preview for pipecd-site canceled.
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues were identified, and all supplied assessments indicate approval readiness.
Review effort: Lite
Findings: None
What changed in this PR
This PR prevents unsupported JSON/HCL event-watcher replacements from truncating files by rejecting them during validation and before writes.
Changes:
- Adds replacement validation and runtime safeguards.
- Adds regression tests for invalid replacements and truncation.
- Updates documentation and
.gitignore.
| File | Summary |
|---|---|
pkg/configv1/event_watcher.go |
Adds v1 replacement validation. |
pkg/configv1/event_watcher_test.go |
Tests invalid replacement configurations. |
pkg/configv1/application.go |
Validates application event watchers. |
pkg/configv1/application_test.go |
Tests application-level validation. |
pkg/config/event_watcher.go |
Adds legacy replacement validation. |
pkg/config/application.go |
Validates legacy application event watchers. |
pkg/app/pipedv1/eventwatcher/eventwatcher.go |
Prevents unsupported writes and reports failures. |
pkg/app/pipedv1/eventwatcher/eventwatcher_test.go |
Tests files are not truncated. |
pkg/app/piped/eventwatcher/eventwatcher.go |
Applies equivalent legacy safeguards. |
pkg/app/piped/eventwatcher/eventwatcher_test.go |
Tests legacy safeguards. |
docs/content/en/docs-v1.0.x/user-guide/managing-application/configuration-reference.md |
Documents supported replacement formats. |
.gitignore |
Ignores WORKABLE.md. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
Adding the check to the application spec validation has a wider effect than the event watcher. I loaded the repro config from #7394 with LoadApplication on master and on this branch. On master it loads, and here it fails with "invalid event watcher handler config: replacement has an unsupported jsonField" as the error.
On that error the trigger reports the app as INVALID_CONFIG and skips it, so an app that only declares a jsonField replacement stops being deployed, even if no matching event has ever arrived. The event watcher also skips an app whose config fails to load, so for pipedv1 the new check in commitFiles is never reached and the event is never marked FAILURE.
If that's intended, it's worth stating in the user-facing section, since it's broader than the event watcher returning an error. If not, the validation at the top of commitFiles already stops the truncation on its own, before any file is written.
Separately, the .gitignore change adds WORKABLE.md, which looks like a local file.
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
|
Kindly assist in reviewing, as I have made the necessary changes in the commit 0a3dd44 |
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
Thanks, 0a3dd44 covers both points. The #7394 config loads again with LoadApplication on this head (on 9474ff9 it failed with the jsonField error), and when I remove the checks in commitFiles (the validation at the top and the jsonField and HCLField returns), the new truncation test fails, so those checks are what keep the file intact.
The same rejection is still in EventWatcherEvent.Validate in both config packages, which runs when LoadEventWatcher reads the deprecated event watcher files under .pipe/. I loaded a .pipe/ file with one yamlField event and one jsonField event. On master both events load. On this branch the whole file fails with "event "image-update": replacement has an unsupported jsonField" as the error.
In both pipeds a LoadEventWatcher error is logged and the watcher moves on to the next repository. For that repository, the yamlField event in the same file and the eventWatcher entries in its application configs are skipped too, on every run until the file is changed. Those .pipe/ events already reach commitFiles through updateValues, where the new check stops the truncation and skips only the event with the jsonField. Leaving the rejection out of EventWatcherEvent.Validate would keep the effect to that one event, as it now is for the application config.
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
|
Kindly assist in reviewing, as I have made the necessary changes in the commit 188869d |
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
Re-checked at 188869d. The legacy loading point is fixed: EventWatcherEvent.Validate in both config packages matches master again, so a .pipe/ file with a jsonField event loads as before, and the tests in both event watcher packages and both config packages pass.
Two smaller things on this head.
-
EventWatcherConfig.ValidateandEventWatcherHandlerConfig.Validatehave no callers since the application spec stopped calling them in 0a3dd44, so they can be removed from both config packages. -
The new check at the top of commitFiles counts regex as a field, so a replacement with both yamlField and regex now fails with "replacement has multiple fields". I confirmed that with a quick probe:
EventWatcherEvent.Validateaccepts that replacement, as it does on master, andEventWatcherReplacement.Validaterejects it. On master commitFiles applies the yamlField and ignores the regex, so a config with both goes from working to failing every event. If rejecting that pair is intended, a line in the user-facing note would help users who hit it. If not, counting regex separately from the three field types keeps the current behaviour.
|
The error has been fixed in commit 9987a3e |
|
sign off commits @JbravoI |
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
|
@harshrajdebug Commits now signed off. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7409 +/- ##
==========================================
+ Coverage 30.35% 30.95% +0.59%
==========================================
Files 601 602 +1
Lines 52620 52900 +280
==========================================
+ Hits 15975 16377 +402
+ Misses 35123 34972 -151
- Partials 1522 1551 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Ewuji John <johnseyi51@gmail.com>
harshrajdebug
left a comment
harshrajdebug
left a comment
There was a problem hiding this comment.
Re-checked at 4c66cb2. Both points from the last round are covered: the uncalled config-level Validate methods are gone (EventWatcherHandlerConfig.Validate in both config packages, plus EventWatcherConfig.Validate in configv1), and EventWatcherReplacement.Validate no longer counts regex as a field, so a replacement with both yamlField and regex passes the check at the top of commitFiles and gets the yamlField applied, as on master. The new TestEventWatcherReplacementValidate cases cover that pair, and the config and event watcher tests in both pipeds pass on this head. Looks good to me.
What this PR does:
jsonFieldandHCLFieldevent-watcher replacements from reaching a file write; they now return an explicit unsupported-format error.jsonFieldandHCLFieldduring event-watcher configuration validation.yamlFieldandregex.Why we need it:
The event watcher previously accepted JSON/HCL replacement configuration but did not implement either path. When one was processed, the code wrote a nil byte slice to the target file, truncating it to zero bytes before committing the change. This change turns that destructive behavior into a clear validation or runtime error and documents the supported configuration accurately.
Which issue(s) this PR fixes:
Fixes #7394
Does this PR introduce a user-facing change?: Yes.
jsonFieldorHCLFieldnow fail with an explicit unsupported-field error instead of creating a commit that empties the target file.yamlFieldandregexreplacements are unaffected.jsonFieldorHCLField. Those formats were documented but not implemented safely.jsonField/HCLFieldwith a supportedyamlFieldorregexreplacement where practical. Otherwise, remove the event-watcher replacement until JSON/HCL support is implemented.Screenshots/Videos (for documentation or website changes):
Not applicable. This documentation change removes unsupported configuration fields from a reference table and has no visual UI change.
Verification:
Both targeted tests pass.