Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECS] Fix unexpected diff of Plan Preview #5306

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Nov 1, 2024

What this PR does:

  • Clone manifests in DriftDetection not to modify the original ones in AppManifestsCache.

NOTE: I'll fix Lambda too, it has the same problem.

Why we need it:

  • Without this PR, unexpected diff appears in ECS PlanPreview.

    e.g. Subnets, Essential, and Protocol are unexpected.
    image

  • The cause is that ignoreAndSortParameters() in ECS DriftDetection unintentionally overwrites ECS manifests in AppManifestsCache.

    • PlanPreview loads manifests from AppManifestsCache. The cache is shared with DriftDetection.
      oldManifests, err = b.loadECSManifests(ctx, *app, runningDSP)
    • It is values of pointer targets in the cache that are overwritten. Cache values themselves are not overwritten.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.

Project coverage is 25.23%. Comparing base (c842a89) to head (48ea969).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/piped/driftdetector/ecs/detector.go 86.95% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5306      +/-   ##
==========================================
+ Coverage   25.02%   25.23%   +0.20%     
==========================================
  Files         447      444       -3     
  Lines       47520    47480      -40     
==========================================
+ Hits        11892    11980      +88     
+ Misses      34690    34559     -131     
- Partials      938      941       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 👍

@t-kikuc t-kikuc merged commit d7f4369 into master Nov 1, 2024
18 checks passed
@t-kikuc t-kikuc deleted the fix-ecs-drift-manifests branch November 1, 2024 06:03
github-actions bot pushed a commit that referenced this pull request Nov 7, 2024
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
ffjlabo added a commit that referenced this pull request Nov 7, 2024
* Add event context (#5295)

* Add contexts to the RegisterEventRequest

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add contexts to model.Event

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Store event context in Control Plane

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add trailers when commiting on event watcher

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Fix for failed build

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

---------

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Add docs for pipectl event register --contexts on the event watcher usage page (#5299)

* Add docs for pipectl event register --contexts on the event watcher usage page

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Fix docs

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Fix command

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

---------

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Clone manifests not to modify original manifests (#5306)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Lambda: clone manifests not to modify original manifests (#5308)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Skip commit and push when no replacement happens in EventWatcher (#5310)

* fix: skip push if no diff exist

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* modify message

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* refactor: use noChange variable

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Execute git clean partially when drift detection for every app is done (#5312)

* Execute git clean partially when drift detection for every app is done

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Rename to CleanPath

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

* Add test for the outside dir pattern

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

---------

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Update RELEASE and docs to v0.49.3 (#5315)

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

---------

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com>
Co-authored-by: Tetsuya Kikuchi <97105818+t-kikuc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants