Skip to content

add: function to return machine pool labels #15

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

Merged

Conversation

VanillaSpoon
Copy link
Contributor

Issue link

Adding this function to use when updating machinepool tests for this pr

What changes have been made

Addition of a label that returns if a machinepool contains a specified label key.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@VanillaSpoon VanillaSpoon removed the request for review from anishasthana November 20, 2023 14:47
Copy link
Collaborator

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

/lgtm

@sutaakar
Copy link
Contributor

@VanillaSpoon I am not much a fan of test functions returning bool.
What is the use case you want to use it in?

@VanillaSpoon
Copy link
Contributor Author

Hi @sutaakar,
Thanks for the question. I was hoping to use the function with this pr. Checking for the existence of a machinepool with a label key matching the appwrapper used to scale up, before and after.

Although, if there is a preferred method, I am happy to update :)

@sutaakar
Copy link
Contributor

@VanillaSpoon The problem with bool comparison like:

	test.Expect(GetMachinePools(test, connection)).
		Should(ContainElement(WithTransform(
			func(machinePool *cmv1.MachinePool) bool {
				return MachinePoolHasLabelKey(machinePool, "test-instascale")
			},
			BeFalse(),
		)))

is that in case of failure you get error message like "true is not false", which doesn't say much.

Better approach is to test for specific value.
So in this case it should be better to check list of keys, like (code untested):

	test.Expect(GetMachinePools(test, connection)).
		Should(ContainElement(WithTransform(MachinePoolLabelKeys, ContainElement("test-instascale"))))

while MachinePoolLabelKeys will return a slice of keys.

What do you think?

@VanillaSpoon
Copy link
Contributor Author

@VanillaSpoon The problem with bool comparison like:

	test.Expect(GetMachinePools(test, connection)).
		Should(ContainElement(WithTransform(
			func(machinePool *cmv1.MachinePool) bool {
				return MachinePoolHasLabelKey(machinePool, "test-instascale")
			},
			BeFalse(),
		)))

is that in case of failure you get error message like "true is not false", which doesn't say much.

Better approach is to test for specific value. So in this case it should be better to check list of keys, like (code untested):

	test.Expect(GetMachinePools(test, connection)).
		Should(ContainElement(WithTransform(MachinePoolLabelKeys, ContainElement("test-instascale"))))

while MachinePoolLabelKeys will return a slice of keys.

What do you think?

Ooohhh I see! Thanks for the clarification, I can see how that's a hindrance.

I love it :) I'll make the adjustments now

@openshift-ci openshift-ci bot removed the lgtm label Nov 21, 2023
@VanillaSpoon VanillaSpoon changed the title add: function to check if the machine pool contains a specific label add: function to return machine pool labels Nov 21, 2023
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

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

/lgtm

@Fiona-Waters
Copy link
Collaborator

/lgtm

Copy link

openshift-ci bot commented Nov 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sutaakar, VanillaSpoon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 9c38a08 into project-codeflare:main Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants