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

Multiple minor corrections #6404

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

Conversation

rickbrouwer
Copy link
Contributor

@rickbrouwer rickbrouwer commented Dec 4, 2024

This PR is a collection of some minor corrections:

  • Fix namespace spelling error
  • Remove period so all messages have no period
  • Remove deprecated test in Pulsar scaler test
  • Remove unnecessary optional when default is set in various scalers
  • Adjust the OpenSSF Scorecard badge link so the Clomonitor regexp sees it

Checklist

@rickbrouwer rickbrouwer requested a review from a team as a code owner December 4, 2024 09:32
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Remove unnecessary optional when default is set in various scalers

TBH I not sure about this, we might use these tags to generate schema and we would like to capture the optional there. CC @wozniakjan

@rickbrouwer rickbrouwer changed the title Corrections Multiple minor corrections Dec 4, 2024
@rickbrouwer
Copy link
Contributor Author

rickbrouwer commented Dec 4, 2024

Remove unnecessary optional when default is set in various scalers

TBH I not sure about this, we might use these tags to generate schema and we would like to capture the optional there. CC @wozniakjan

That's good to know indeed. From TypedConfig optional doesn't do much if it has also a default since a value is set from the default if it's not filled.
Reference:

if !exists && params.Default != "" {
exists = true
valFromConfig = params.Default
}

If there is a good reason, I might even have to make corrections the other way (so add optional where it hasn't been done because of a default)

Signed-off-by: Rick Brouwer <[email protected]>
@wozniakjan
Copy link
Member

wozniakjan commented Dec 4, 2024

TBH I not sure about this, we might use these tags to generate schema and we would like to capture the optional there

my logic was that a field with default is always inherently optional. Default works in a way that when the field is not specified in trigger metadata, KEDA will use the default value. @zroubalik, @rickbrouwer can you think of a situation where a field that is not optional but has default value is useful?

If there is a good reason, I might even have to make corrections the other way

I won't object, but I am leaning towards your current approach where the extra optional are removed. Imho optional is redundant for fields with default.

@rickbrouwer
Copy link
Contributor Author

rickbrouwer commented Dec 4, 2024

I believe that if a field has a default, it is also optional. After all, if the field is mandatory and has a default value and you want this value, you can choose not to specify this value. That automatically makes the field optional.

I would also like to choose my current approach.

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Nice job! Apart from the discussion about optional, just a small nit

ScaleTargetNoSubresourceMsg = "Target resource doesn't expose /scale subresource"
ScaleTargetNoSubresourceMsg = "Target resource doesn't expose / scale subresource"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this change, the resource is /scale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake! I corrected it right away.

Signed-off-by: Rick Brouwer <[email protected]>
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.

4 participants