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 functional tests #262

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Add functional tests #262

merged 1 commit into from
Jan 9, 2025
+2,634 −147

Conversation

omersch381
Copy link
Contributor

No description provided.

@omersch381 omersch381 requested a review from beagles December 16, 2024 10:20
@openshift-ci openshift-ci bot requested a review from frenzyfriday December 16, 2024 10:21
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omersch381

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

@omersch381 omersch381 marked this pull request as draft December 16, 2024 10:21
@omersch381 omersch381 removed the request for review from frenzyfriday December 16, 2024 10:22
@omersch381 omersch381 force-pushed the add_func_tests branch 2 times, most recently from 1723d0f to 12d550d Compare December 16, 2024 15:48
@omersch381 omersch381 changed the title WIP Add functional tests Add functional tests Dec 16, 2024
@omersch381 omersch381 marked this pull request as ready for review December 16, 2024 15:49
@openshift-ci openshift-ci bot requested review from viroel and weinimo December 16, 2024 15:50
omersch381 added a commit to omersch381/release that referenced this pull request Dec 16, 2024
@omersch381
Add functional job for openstack-k8s-operators/designate-operator
6956e12 
Updated unit job to functional job, Need by [1]

[1] openstack-k8s-operators/designate-operator#262
@omersch381 omersch381 mentioned this pull request Dec 16, 2024
Merged
omersch381 added a commit to omersch381/release that referenced this pull request Dec 17, 2024
@omersch381
Add functional job for openstack-k8s-operators/designate-operator
8ebc899 
Updated unit job to functional job, Need by [1]

[1] openstack-k8s-operators/designate-operator#262
omersch381 added a commit to omersch381/release that referenced this pull request Dec 17, 2024
@omersch381
Add functional job for openstack-k8s-operators/designate-operator
deb38c2 
Updated unit job to functional job, Need by [1]

[1] openstack-k8s-operators/designate-operator#262
openshift-merge-bot bot pushed a commit to openshift/release that referenced this pull request Dec 19, 2024
@omersch381
Add functional job for openstack-k8s-operators/designate-operator (#5…
b45b935 
…9930)

Updated unit job to functional job, Need by [1]

[1] openstack-k8s-operators/designate-operator#262
yingzhanredhat pushed a commit to yingzhanredhat/release that referenced this pull request Dec 24, 2024
@omersch381 @yingzhanredhat
Add functional job for openstack-k8s-operators/designate-operator (op…
b10a16b 
…enshift#59930)

Updated unit job to functional job, Need by [1]

[1] openstack-k8s-operators/designate-operator#262
beagles
beagles reviewed Jan 7, 2025
View reviewed changes
api/v1beta1/conditions.go Outdated
// DesignateRabbitMqTransportURLReadyCondition Status=True condition which indicates if the RabbitMQ TransportURLUrl is ready
DesignateRabbitMqTransportURLReadyCondition condition.Type = "DesignateRabbitMqTransportURLReady"
// RabbitMqTransportURLReadyCondition Status=True condition which indicates if the RabbitMQ TransportURLUrl is ready
RabbitMqTransportURLReadyCondition condition.Type = "RabbitMqTransportURLReady"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future reference:a PR that adds tests probably should avoid changing other code. I think it's a decent change in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I changed it because I wanted it to match:
https://github.com/openstack-k8s-operators/lib-common/blob/d172b3ac0f4ee7d91b54be6efe312bdf1f012ee2/modules/common/condition/conditions.go#L313

before I changed it, some tests failed as that condition module didn't have DesignateRabbitMqTransportURLReadyCondition
In that matter, the tests helped me improve the non-testing code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you are right. In fact it seems we should not be redefining these and instead use the conditions and messages from lib-common

beagles
beagles reviewed Jan 7, 2025
View reviewed changes
api/v1beta1/conditions.go Outdated
// DesignateRabbitMqTransportURLReadyInitMessage
DesignateRabbitMqTransportURLReadyInitMessage = "DesignateRabbitMqTransportURL not started"
// RabbitMqTransportURLReadyInitMessage
RabbitMqTransportURLReadyInitMessage = "RabbitMqTransportURL not started"
Copy link
Collaborator

@beagles beagles Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since you're changing them anyways, can you check the messages this against messages in other operators like nova? These were copy/pasted early on without too much critical thought and the text messages aren't super helpful as they refer to constant names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a rabbitmq condition in nova:
~/nova-operator main > rg 'RabbitMq' | rg condition
~/nova-operator main >
however, I did find this line https://github.com/openstack-k8s-operators/lib-common/blob/d172b3ac0f4ee7d91b54be6efe312bdf1f012ee2/modules/common/condition/conditions.go#L313

beagles
beagles reviewed Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@beagles beagles left a comment

Choose a reason for hiding this comment

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

Good work! Just a question about the message text.

@omersch381
Copy link
Contributor Author

hmm the only change I did was replacing designatev1beta.RabbitMqTransportURLReadyCondition with condition.RabbitMqTransportURLReadyCondition and it is not even used in the test.

@omersch381
Copy link
Contributor Author

/retest functional

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 8, 2025

@omersch381: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test designate-operator-build-deploy-kuttl

/test functional

/test images

/test precommit-check

Use /test all to run all jobs.

In response to this:

/retest functional

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@omersch381
Copy link
Contributor Author

/test functional

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 9, 2025 •
edited
Loading

@omersch381: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit abdf61b link true /test unit

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@beagles
Copy link
Collaborator

beagles commented Jan 9, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 9, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 4a66d30 into openstack-k8s-operators:main Jan 9, 2025
5 checks passed
@omersch381 omersch381 deleted the add_func_tests branch January 14, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beagles beagles beagles left review comments

Assignees

@beagles beagles

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omersch381 @beagles @openshift-merge-robot