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

Add calling annotateConfigHash and refactor unmarshaling deploy target #5421

Merged
merged 5 commits into from
Dec 16, 2024

Conversation

Warashi
Copy link
Contributor

@Warashi Warashi commented Dec 16, 2024

What this PR does:

  1. add calling annotateConfigHash
  2. refactor the unmarshaling of the deploy target config

Why we need it:

  1. we want to refresh workloads when the config changes

Which issue(s) this PR fixes:

Part of #4980

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: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 48.27586% with 15 lines in your changes missing coverage. Please review.

Project coverage is 25.98%. Comparing base (d5ba3d2) to head (bc84766).

Files with missing lines Patch % Lines
...app/pipedv1/plugin/kubernetes/deployment/server.go 0.00% 12 Missing ⚠️
...pp/pipedv1/plugin/kubernetes/config/application.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5421      +/-   ##
==========================================
+ Coverage   25.96%   25.98%   +0.01%     
==========================================
  Files         451      451              
  Lines       48793    48816      +23     
==========================================
+ Hits        12669    12683      +14     
- Misses      35118    35126       +8     
- Partials     1006     1007       +1     

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

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
@Warashi Warashi marked this pull request as ready for review December 16, 2024 02:29
Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

Almost all LGTM! I left just one nit comment.

Comment on lines +91 to +95
if tt.expectedErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

[nit]

Suggested change
if tt.expectedErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
assert.Equal(t, tt.expectedErr, err != nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my code provides more information when debugging the codes.
Your suggested code works fine when the test passes, but there is no information about the error when the test fails.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

OK, there was a reason. got it. I also agree with that reason.

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

🚀

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.

Here you go 👍

@khanhtc1202 khanhtc1202 merged commit 149078a into master Dec 16, 2024
18 checks passed
@khanhtc1202 khanhtc1202 deleted the k8s-plugin-annotate-config-hash-3 branch December 16, 2024 10:21
@github-actions github-actions bot mentioned this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants