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

fix(diff): support auto fields #136

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Conversation

samugi
Copy link
Member

@samugi samugi commented Aug 28, 2024

Summary

fix(diff): support auto fields

call the new utils function FillPluginsDefaultsAutoFields from  go-kong
to ensure auto fields are considered when doing the diff

fix(crud): detect operation correctly

fixes a bug that was introduced in
e72f4c2de6609998d5e5671e14a9f7c0cd6a4c3e where noops were still detected
as updates. That happened because of a check happening in plugin.go that
detected the crud operation based on a diff.
When done without considering default/auto fields, this check would see
differences where there were none. This resulted in unnecessary updates
to the DB and wrong diff strings during `sync` and `diff`.

This commit adds defaults/auto fields to the configuration used for that
diff.

Full changelog

  • [Implement ...]
  • [Fix ...]

Issues resolved

KAG-5210

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

@samugi samugi force-pushed the fix/support-auto-fields-diff branch from 582099a to b1914e8 Compare August 28, 2024 17:42
@samugi samugi marked this pull request as draft August 28, 2024 20:23
@samugi samugi force-pushed the fix/support-auto-fields-diff branch 5 times, most recently from 7d06537 to 1d145a3 Compare August 29, 2024 09:31
@samugi

This comment was marked as outdated.

@randmonkey

This comment was marked as outdated.

@samugi

This comment was marked as outdated.

@samugi

This comment was marked as outdated.

call the new utils function FillPluginsDefaultsAutoFields from  go-kong
to ensure auto fields are considered when doing the diff
fixes a bug that was introduced in
e72f4c2 where noops were still detected
as updates. That happened because of a check happening in plugin.go that
detected the crud operation based on a diff.
When done without considering default/auto fields, this check would see
differences where there were none. This resulted in unnecessary updates
to the DB and wrong diff strings during `sync` and `diff`.

This commit adds defaults/auto fields to the configuration used for that
diff.
@samugi samugi force-pushed the fix/support-auto-fields-diff branch from 1d145a3 to ad71783 Compare September 2, 2024 07:20
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 29.97%. Comparing base (25ccc04) to head (72266da).

Files with missing lines Patch % Lines
pkg/types/plugin.go 0.00% 8 Missing ⚠️
pkg/diff/diff.go 0.00% 7 Missing ⚠️
pkg/types/core.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
- Coverage   29.99%   29.97%   -0.03%     
==========================================
  Files         106      106              
  Lines       12571    12581      +10     
==========================================
  Hits         3771     3771              
- Misses       8340     8350      +10     
  Partials      460      460              

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

pkg/diff/diff.go Outdated Show resolved Hide resolved
@samugi samugi marked this pull request as ready for review September 3, 2024 06:56
@randmonkey randmonkey merged commit 0b1b75f into main Sep 3, 2024
18 checks passed
@randmonkey randmonkey deleted the fix/support-auto-fields-diff branch September 3, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants