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.

Sign up for GitHub

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

Jump to bottom

Add missing controller test for PoolName #1069

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Add missing controller test for PoolName #1069

merged 2 commits into from
Nov 22, 2024

Conversation

shajmakh
Copy link
Member

@shajmakh shajmakh commented Nov 13, 2024 •
edited
Loading

add missing test for empty pool name and extra verification for Available condition

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2024
@shajmakh
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
@shajmakh
Copy link
Member Author

/unhold
the value for this is for OCP only and in that context shouldn't be blocked. HCP cases are anyway broken and needs another PR to fix: #1070

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
ffromani
ffromani reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

the additions here look good. IIRC, in #1064 we fixed the bug on which the DS was created, but immediately removed because the bug in the dangling pkg. I'd like a test which specifically check for this condition. Is the second commit targeting explicitely that case? it seems related but not too explicit.

@shajmakh
Copy link
Member Author

the additions here look good. IIRC, in #1064 we fixed the bug on which the DS was created, but immediately removed because the bug in the dangling pkg. I'd like a test which specifically check for this condition. Is the second commit targeting explicitely that case? it seems related but not too explicit.

Yes the intended test is in the second commit. The problem appeared in HCP but the test doesn't run on that platform, and would need fixes anyway. I haven't tested if the bug occurs on OCP manually, will do the checks and update the code as needed.

@ffromani
Copy link
Member

the additions here look good. IIRC, in #1064 we fixed the bug on which the DS was created, but immediately removed because the bug in the dangling pkg. I'd like a test which specifically check for this condition. Is the second commit targeting explicitely that case? it seems related but not too explicit.

Yes the intended test is in the second commit. The problem appeared in HCP but the test doesn't run on that platform, and would need fixes anyway. I haven't tested if the bug occurs on OCP manually, will do the checks and update the code as needed.

fair enough. Please add a comment above the addition explicitely mentioning that PR, mentioning the bug we had and how this check helps in this area. Then I think I can move on and merge

@shajmakh
Copy link
Member Author

shajmakh commented Nov 20, 2024 •
edited
Loading

/hold
for #1083 to be merged first as it will enable the tests on hypershift. Also to avoid rebase conflicts. this is easier to add later.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024
@ffromani
Copy link
Member

/hold

let's wait for #1071 to go in

@ffromani
Copy link
Member

/hold cancel

#1071 merged

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024
ffromani
ffromani reviewed Nov 20, 2024
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/lgtm

/retest-required

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@ffromani
Copy link
Member

can we please rebase on top of current main branch?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024
@shajmakh
Copy link
Member Author

Following further examination, the bug fixed in #1064 can't be caught in the controller tests but in the e2e instead, because we need to look at the readiness of the ds not only the creation. This is covered by the e2e/serial e2e/rte and e2e/install. I adjusted the second commit message.

@shajmakh
Copy link
Member Author

/hold
for #1083 to avoid uneeded complex rebase conflicts

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2024
@shajmakh
ctrl: test empty string as PoolName
dfcd9d4 
Add missing test for PoolName as kempty string.

Signed-off-by: Shereen Haj <[email protected]>
@shajmakh
ctrl test: verify Avialable condition is true
35c11a1 
Extend the current test to verify the available condition of the
operator for extra verification.

Signed-off-by: Shereen Haj <[email protected]>
@shajmakh
Copy link
Member Author

/unhold

@shajmakh shajmakh changed the title Add missing controller tests for PoolName Add missing controller test for PoolName Nov 21, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2024
ffromani
ffromani reviewed Nov 21, 2024
View reviewed changes
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024
@ffromani
Copy link
Member

/retest-required

@shajmakh
Copy link
Member Author

e2e-install failure aligns with the new default behavior
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2024
@shajmakh
Copy link
Member Author

shajmakh commented Nov 22, 2024 •
edited
Loading

/retest
to verify if we get the same failure

@shajmakh
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 9aa6430 into openshift-kni:main Nov 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffromani ffromani ffromani left review comments

@swatisehgal swatisehgal Awaiting requested review from swatisehgal

Assignees

@ffromani ffromani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shajmakh @ffromani