Skip to content

Conversation

@itzikb-redhat
Copy link
Contributor

Check that a minimal flavor pass
Check that a flavor without RAM or VCPUs or with value 0 fails
Add validations for RAM and VCPUs to have a minimum
Check that a flavor with a description greater than 1024 fails
Move testCredentials to common_test

Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

Some nits, but as far as I'm concerned if the added tests pass and it passes make lint it's good to go 👌

)

// Create a random string of length n
func generateResouceName(prefix string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need this, btw, as each test runs in its own namespace. (Also typo in the name, btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

obj.Name = generateResouceName(flavorPrefix)
obj.Namespace = namespace.Name
return obj
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing blank line below. Surprised go fmt didn't complain about that 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pierreprinetti
Copy link
Member

go-fmt should succeed after a rebase

Check that a minimal flavor pass
Check that a flavor without RAM or VCPUs or with value 0 fails
Add validations for RAM and VCPUs to have a minimum
Check that a flavor with a description greater than 1024 fails
Move testCredentials to common_test
Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

This looks great. Lets ship it!

Copy link
Collaborator

@mandre mandre left a comment

Choose a reason for hiding this comment

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

LGTM

@mandre mandre merged commit 744d859 into k-orc:main Dec 12, 2024
3 checks passed
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