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 testLogBug and changeSomeVariableName #6420

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ctccxxd
Copy link

@ctccxxd ctccxxd commented Dec 12, 2024

  1. Change variableName in code or annotation.
  2. Fix test case log bug.
  3. upgrade crypto to v0.31.0 to pass code check in GitHub.

Checklist

Fixes #

Relates to #

@ctccxxd ctccxxd requested a review from a team as a code owner December 12, 2024 10:36
@ctccxxd ctccxxd changed the title fix testLogBug and changeSomeVariableName Fix testLogBug and changeSomeVariableName Dec 12, 2024
@ctccxxd
Copy link
Author

ctccxxd commented Dec 12, 2024

Friendly ping @wozniakjan @SpiritZhou @JorTurFer hope you to review and discuss, much thanks.

Comment on lines 234 to 235
var incomingSvGvkr GroupVersionKindResource
incomingSvGvkr, err = ParseGVKR(restMapper, incomingSo.Spec.ScaleTargetRef.APIVersion, incomingSo.Spec.ScaleTargetRef.Kind)
Copy link
Member

Choose a reason for hiding this comment

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

why Sv instead of So?

Copy link
Author

Choose a reason for hiding this comment

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

why Sv instead of So?

Because GVKR represents GroupVersionKindResource, and this is be seen more common and explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Gckr vs Gvkr is clear (and the change is 100% worth), but SO means ScaledObject, that's why my question

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, Gckr vs Gvkr is clear (and the change is 100% worth), but SO means ScaledObject, that's why my question

Haha, an oolong,I have revised. Wait for your review. thanks

go.mod Outdated
Comment on lines 147 to 148
// https://avd.aquasec.com/nvd/cve-2024-45337
golang.org/x/crypto => golang.org/x/crypto v0.31.0
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this to another PR as it's to solve an issue by bumping deps

Copy link
Author

Choose a reason for hiding this comment

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

I'd move this to another PR as it's to solve an issue by bumping deps

ok

Copy link
Member

Choose a reason for hiding this comment

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

I've created this PR to bump the dependency as it has a critical CVE -> #6422

Copy link
Author

@ctccxxd ctccxxd Dec 16, 2024

Choose a reason for hiding this comment

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

I've created this PR to bump the dependency as it has a critical CVE -> #6422

I have revised here, maybe can directly merge this PR. Otherwise, I have to separate this PR.

Copy link
Member

Choose a reason for hiding this comment

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

please rebase your PR with main changes 🙏 sorry for this, I prefer to have an isolated PR to fix the CVE because it's a 9.1 that needs to be trackable IMHO

Copy link
Author

Choose a reason for hiding this comment

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

please rebase your PR with main changes 🙏 sorry for this, I prefer to have an isolated PR to fix the CVE because it's a 9.1 that needs to be trackable IMHO

make sense, both are all ok.

CHANGELOG.md Outdated
@@ -75,6 +75,7 @@ Here is an overview of all new **experimental** features:

- **General**: Centralize and improve automaxprocs configuration with proper structured logging ([#5970](https://github.com/kedacore/keda/issues/5970))
- **General**: Paused ScaledObject count is reported correctly after operator restart ([#6321](https://github.com/kedacore/keda/issues/6321))
- **General**: Revise some variable names and wrong test log([#6420](https://github.com/kedacore/keda/pull/6420))
Copy link
Member

Choose a reason for hiding this comment

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

Let' move this into Other section - as it is not user facing fix. Or we can even remove it from the changelog. I am okay with both.

Copy link
Author

Choose a reason for hiding this comment

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

Let' move this into Other section - as it is not user facing fix. Or we can even remove it from the changelog. I am okay with both.

ok

Copy link
Author

Choose a reason for hiding this comment

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

done, friendly ping @zroubalik @zroubalik , hope for your review, thank you.

@ctccxxd ctccxxd force-pushed the changeSomeVariableNameLog branch from 8dd5470 to f56c05b Compare December 16, 2024 23:58
@ctccxxd
Copy link
Author

ctccxxd commented Dec 17, 2024

done, friendly ping @zroubalik @zroubalik , hope for your review, thank you.

@JorTurFer
Copy link
Member

JorTurFer commented Dec 22, 2024

/run-e2e internal
Update: You can check the progress here

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