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

improved webhook event trigger test #13048

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

pondrejk
Copy link
Contributor

@pondrejk pondrejk commented Nov 7, 2023

Problem Statement

Webhook trigger is blocked by the safemode setting enabled, which is expected as the safemode prevents template rendering which is prerequisite for webhook trigger.

The existing test didn't see this as it was looking just for one specific line in logs, so it passed even though no action was triggered.

Solution

This PR disables the safemode setting for the test so that the task is actually triggers. Also it adds a check for the successful completion of the triggered task.

@pondrejk pondrejk added API Issues and PRs involving the API CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 labels Nov 7, 2023
@pondrejk pondrejk requested review from a team November 7, 2023 09:38
@pondrejk pondrejk self-assigned this Nov 7, 2023
@pondrejk
Copy link
Contributor Author

pondrejk commented Nov 7, 2023

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered

Copy link
Contributor

@jameerpathan111 jameerpathan111 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

I don't completely understand what the setting change does, but it looks good to me. When I originally wrote this test I just was making sure the webhook event was triggering something in the log.

@Griffin-Sullivan
Copy link
Contributor

Also is there a reason this is marked for T2 review also?

@pondrejk pondrejk force-pushed the hook-triggered-assertions branch from 8530ff4 to 7d3c631 Compare November 9, 2023 08:15
@pondrejk
Copy link
Contributor Author

pondrejk commented Nov 9, 2023

@Griffin-Sullivan safemode kind of protects users from doing something destructive with their ERB templates, https://access.redhat.com/documentation/en-us/red_hat_satellite/6.14/html/administering_red_hat_satellite/administration_settings_admin (section A11), it's on by default. T2 was there before, I just kept it

@pondrejk
Copy link
Contributor Author

pondrejk commented Nov 9, 2023

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered

@pondrejk
Copy link
Contributor Author

pondrejk commented Nov 9, 2023

ugh, after move from entities.Webhook to target_sat.api.Webhook this now fails with TypeError: Webhooks.__init__() got multiple values for argument 'server_config'

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered
nailgun: 1029

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered
nailgun: 1029
pod_resources_size: large

Co-authored-by: Omkar Khatavkar <okhatavk@redhat.com>
@pondrejk
Copy link
Contributor Author

pondrejk commented Nov 9, 2023

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered
pod_resources_size: large

1 similar comment
@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered
pod_resources_size: large

@pondrejk
Copy link
Contributor Author

The prt fails with the TypeError as before, seems like it doesn't take into account the nailgun fix by @omkarkhatavkar ?

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/api/test_webhook.py -k triggered
nailgun: 1029
pod_resources_size: large

@omkarkhatavkar
Copy link

The prt fails with the TypeError as before, seems like it doesn't take into account the nailgun fix by @omkarkhatavkar ?

https://github.com/SatelliteQE/robottelo/actions/workflows/update_robottelo_image.yml is failing.

@pondrejk
Copy link
Contributor Author

yay! thanks for the quick turnaround @omkarkhatavkar

@pondrejk pondrejk merged commit 1c05f47 into SatelliteQE:master Nov 10, 2023
5 checks passed
shweta83 pushed a commit to shweta83/robottelo that referenced this pull request Apr 10, 2024
* improved webhook event trigger test

* Update tests/foreman/api/test_webhook.py

Co-authored-by: Omkar Khatavkar <okhatavk@redhat.com>

---------

Co-authored-by: Omkar Khatavkar <okhatavk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 API Issues and PRs involving the API AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants