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

Remove json serialization for notify validation #14847

Merged
merged 2 commits into from Feb 12, 2024
Merged

Remove json serialization for notify validation #14847

merged 2 commits into from Feb 12, 2024

Conversation

dmzoneill
Copy link
Member

@dmzoneill dmzoneill commented Feb 6, 2024

SUMMARY

For the webhook notification, the start body only accepts variables whom transpose to json dict.
E.g : {{job_metadata}} =

{ ..... } 

All other variables such as {{job.id}} which are primitives (int, str, float, etc) do not pass the validation.
They are not dict type and the below check does not allow {{job.scm_reivsion}} etc as requested by #14410

if not isinstance(potential_body, dict):

This patch loosens up this validation to just check for jinja template exception.

related: #14410

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

# _("Webhook body for '{}' should be a json dictionary. Found type '{}'.".format(event, type(potential_body).__name__))
# )
except Exception as exc:
error_list.append(_("Webhook body for '{}' is not valid. The following gace an error ({}).".format(event, exc)))
Copy link
Member

Choose a reason for hiding this comment

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

s/gace/gave/

@chrismeyersfsu
Copy link
Member

Wow, nice find. How did this ever work? I wonder if we expected the user to manually construct a valid json

i.e.

{
  "my_job_revision": "{{ job.scm_revision }}"
}

vs.

{{ job.scm_revision }}

I reckon this would make it so we support a wider range of webhook servers on the other end .. but that isn't true because we expect the payload to be a JSON. So it's not like the user can go construction xml for a soap request (warning, I don't know what a soap request really looks like).

<xml>
<my_job_revision>{{ job.scm_revision }}</my_job_revision>
</xml>

/end ramble

@dmzoneill dmzoneill self-assigned this Feb 7, 2024
@dmzoneill dmzoneill merged commit 56b6a07 into devel Feb 12, 2024
21 checks passed
@dmzoneill dmzoneill deleted the 14410 branch February 12, 2024 15:43
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.

None yet

2 participants