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 DataPlane log #847

Merged
merged 1 commit into from
Nov 5, 2024
Merged

fix: diff DataPlane log #847

merged 1 commit into from
Nov 5, 2024

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Nov 5, 2024

What this PR does / why we need it:

3rd parameter of log functions (including Trace) is rawObj, not string. This has been reported by user in #522 (comment).

Current code yields:

unexpected type processed for trace logging: string, this is a bug!

This PR fixes it.

@pmalek pmalek added this to the KGO v1.5.x milestone Nov 5, 2024
@pmalek pmalek self-assigned this Nov 5, 2024
@pmalek pmalek requested a review from a team as a code owner November 5, 2024 09:48
@pmalek pmalek enabled auto-merge (squash) November 5, 2024 09:48
@czeslavo
Copy link
Contributor

czeslavo commented Nov 5, 2024

Can we make the log functions type-safe to ensure the rawObj is client.Object to avoid such situations? BTW looking into the source code, rawObj is unused. Are these logging helpers all right?

@pmalek pmalek merged commit 0ecbe7f into main Nov 5, 2024
21 checks passed
@pmalek pmalek deleted the fix-dataplane-deployment-log branch November 5, 2024 10:01
@pmalek
Copy link
Member Author

pmalek commented Nov 5, 2024

Can we make the log functions type-safe to ensure the rawObj is client.Object to avoid such situations?

We can't do this today (without refactoring) as not all objects passed as rawObj are client.Objects (some of those only implement this interface for the pointer type and are passed by value).

BTW looking into the source code, rawObj is unused. Are these logging helpers all right?

This has been changed in #727. Currently the rawObj is indeed unused as that information was being duplicated. Let me try to propose something that will fix this issue.

@pmalek
Copy link
Member Author

pmalek commented Nov 5, 2024

@czeslavo PTAL at #848

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