Skip to content
This repository was archived by the owner on Dec 17, 2025. It is now read-only.

fix: Change wrong log level in cluster.go openAPISchema, gvkParser#430

Merged
leoluz merged 3 commits intoargoproj:masterfrom
dllegru:425_error_creating_gvk_parser
Aug 2, 2022
Merged

fix: Change wrong log level in cluster.go openAPISchema, gvkParser#430
leoluz merged 3 commits intoargoproj:masterfrom
dllegru:425_error_creating_gvk_parser

Conversation

@dllegru
Copy link
Copy Markdown
Contributor

@dllegru dllegru commented Jul 13, 2022

Fixes #425

Change wrong log level from Error to Warning.

@leoluz
Copy link
Copy Markdown
Contributor

leoluz commented Jul 25, 2022

@dllegru The reason why I added the log as Error is because the log interface doesn't have the Warning method. This is why your PR is failing with error:

pkg/cache/cluster.go:733: c.log.Warning undefined (type logr.Logger has no field or method Warning)

We need to define and implement this method. Would you be willing to go ahead and update your PR with this implementation?

@dllegru
Copy link
Copy Markdown
Contributor Author

dllegru commented Jul 29, 2022

@dllegru The reason why I added the log as Error is because the log interface doesn't have the Warning method. This is why your PR is failing with error:

pkg/cache/cluster.go:733: c.log.Warning undefined (type logr.Logger has no field or method Warning)

We need to define and implement this method. Would you be willing to go ahead and update your PR with this implementation?

I'm sorry @leoluz but my current experience with Golang at the moment is close to 0, so can't help much further at this stage.

@leoluz
Copy link
Copy Markdown
Contributor

leoluz commented Jul 29, 2022

@dllegru No problem.. I dug a bit and realized that the problem is actually caused by the library we use that doesn't provide a log.Warning method.
If you can just change the log to Info in your PR it will work and it should be fine to merge. Also, if you are updating your PR pls don't forget to signoff your commits (git commit -sm 'commit message') or the DCO check will fail.
Thank you.

@dllegru
Copy link
Copy Markdown
Contributor Author

dllegru commented Aug 2, 2022

Hi @leoluz, thanks for digging into it and sharing the documentation, it's very insightftul.
I've changed to c.log.Info as suggested although the tests now fail with:

Error: pkg/cache/cluster.go:669:18: cannot use e (type *kube.CreateGVKParserError) as type string in argument to c.log.Info
Error: pkg/cache/cluster.go:733:13: cannot use e (type *kube.CreateGVKParserError) as type string in argument to c.log.Info

It seems the problem is in using string format var e cannot be used with c.log.Info. I'm sorry can't provide much more help at this moment due my lack of knowledge with Go.

Copy link
Copy Markdown
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

Please take a look at the change suggestions to fix the build.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 2, 2022

Codecov Report

Merging #430 (7bcf356) into master (67ddccd) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   54.42%   54.42%           
=======================================
  Files          41       41           
  Lines        3107     3107           
=======================================
  Hits         1691     1691           
  Misses       1249     1249           
  Partials      167      167           
Impacted Files Coverage Δ
pkg/cache/cluster.go 54.36% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67ddccd...7bcf356. Read the comment docs.

@leoluz
Copy link
Copy Markdown
Contributor

leoluz commented Aug 2, 2022

CI/Test job is now passing. Thank you.
@dllegru You still need to signoff your commits for me to approve/merge. You can do so by running the following commands in your terminal:

git rebase HEAD~3 --signoff
git push --force-with-lease origin 425_error_creating_gvk_parser

dllegru and others added 3 commits August 2, 2022 18:19
Fixes #423

Signed-off-by: Daniel Urgell <urgell.d@gmail.com>
Signed-off-by: Daniel Urgell <urgell.d@gmail.com>
Signed-off-by: Daniel Urgell <urgell.d@gmail.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Aug 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dllegru
Copy link
Copy Markdown
Contributor Author

dllegru commented Aug 2, 2022

CI/Test job is now passing. Thank you. @dllegru You still need to signoff your commits for me to approve/merge. You can do so by running the following commands in your terminal:

git rebase HEAD~3 --signoff
git push --force-with-lease origin 425_error_creating_gvk_parser

Many thanks to you @leoluz!
I've rebased all the commits, as you pointed I had a missing sign-off message (thanks for the command) in a previous commit and also the email address was wrongly set-up.

I think finally all looks good now! 🥳

@dllegru dllegru requested a review from leoluz August 2, 2022 16:25
Copy link
Copy Markdown
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@leoluz leoluz merged commit da66819 into argoproj:master Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error="error creating gvk parser: duplicate entry for /v1, Kind=APIGroup"

3 participants