tektoncd / triggers

Event triggering with Tekton!
Apache License 2.0
546 stars 416 forks source link

Follow naming conventions for resourceTemplates #1650

Closed savitaashture closed 8 months ago

savitaashture commented 10 months ago

As per the standards the JSON or YAML keys should follow capitalization but resourcetemplates keys doesn't follow that. Changing resourcetemplates to resourceTemplates

Signed-off-by: Savita Ashture sashture@redhat.com"

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Release Notes

NONE
savitaashture commented 10 months ago

What would happen if some resource already exists with that version? Don't we need a conversion function?

Its not needed because it works for both resourceTemplates and resourcetemplates i have verified

khrm commented 10 months ago

What would happen if some resource already exists with that version? Don't we need a conversion function?

Its not needed because it works for both resourceTemplates and resourcetemplates i have verified

It's working because go default JSON decoder/encoder doesn't care about case or characters like _. So etcd would have two different field names after this pr - older with resourcetemplates and newly created with resourceTemplates.

savitaashture commented 9 months ago

What would happen if some resource already exists with that version? Don't we need a conversion function?

Its not needed because it works for both resourceTemplates and resourcetemplates i have verified

It's working because go default JSON decoder/encoder doesn't care about case or characters like _. So etcd would have two different field names after this pr - older with resourcetemplates and newly created with resourceTemplates.

Yes but I think that should be fine right

dibyom commented 9 months ago

This seems like the right thing to do - as long as this is not a breaking change i.e. the old style works too

savitaashture commented 9 months ago

Hi @dibyom @khrm if PR looks good can you guys review and merge it Thank you

tekton-robot commented 9 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

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: - ~~[OWNERS](https://github.com/tektoncd/triggers/blob/main/OWNERS)~~ [khrm] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment