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

fix(Cal Trigger Node): Update to support v2 webhooks #5331

Merged
merged 5 commits into from
Mar 8, 2023
Merged

fix(Cal Trigger Node): Update to support v2 webhooks #5331

merged 5 commits into from
Mar 8, 2023

Conversation

alishaz-polymath
Copy link
Contributor

This PR adds support for a new Event trigger MEETING_ENDED and fixes the webhooks endpoint (used to be /hooks, now it's /webhooks)

@alishaz-polymath alishaz-polymath changed the title Cal trigger maintenance fix(Cal Trigger Node): Cal trigger maintenance Feb 2, 2023
@n8n-assistant n8n-assistant bot added community Authored by a community member node/improvement New feature or request labels Feb 2, 2023
@Joffcom
Copy link
Member

Joffcom commented Feb 2, 2023

Hey @alishaz-polymath,

The change looks good but if we merge this will it break for self hosted cal users that have not updated? We can work around this with light versioning so it works for new users and old users, I tried to find release notes to find the version change so I can get it implemented but was not successful.

@alishaz-polymath
Copy link
Contributor Author

alishaz-polymath commented Feb 3, 2023 •
edited
Loading

Hey @alishaz-polymath,

The change looks good but if we merge this will it break for self hosted cal users that have not updated? We can work around this with light versioning so it works for new users and old users, I tried to find release notes to find the version change so I can get it implemented but was not successful.

Hey @Joffcom
The endpoint changes (/hooks => /webhooks) are part of our private API repo. And since the API repo versioning is not the same as the calcom repo versioning, it might be trickier than we think.
The endpoint change was done around 4-5 months ago while the new Event Trigger was added in https://github.com/calcom/cal.com/releases/tag/v1.9.0
What do you recommend in this case? 🤔

@alishaz-polymath
Copy link
Contributor Author

For what it's worth, there have been major bug fixes, upgrades and feature changes in the past 4-5 months as well, so I'd probably recommend them to upgrade to one of the later versions. Perhaps we can proceed with the changes as they are currently as it's far more stable now anyway.

@alishaz-polymath
Copy link
Contributor Author

Hi @Joffcom
Could you please point me in the direction of the recommended way to version the trigger node in n8n? I am more than willing to set it up in accordance with that 🙏

@alishaz-polymath alishaz-polymath mentioned this pull request Feb 16, 2023
Closed
@Joffcom
Copy link
Member

Joffcom commented Feb 16, 2023

Hey @alishaz-polymath,

That sounds good to me, If you take a look at the NocoDB node you can see the light versioning in action. I know it is such a small change that may not impact anyone but I am a big fan of playing it safe and you never know what users are self hosting.

@Joffcom
Copy link
Member

Joffcom commented Feb 21, 2023

@alishaz-polymath that looks good, I have not had a chance to test it but I will make an individual account on the site in the morning and see if we can get this into 0.217.0 this week.

@alishaz-polymath
Copy link
Contributor Author

@alishaz-polymath that looks good, I have not had a chance to test it but I will make an individual account on the site in the morning and see if we can get this into 0.217.0 this week.

Unfortunately I was unable to test it locally because I keep running into development build errors, even without any changes to the current n8n master branch. I'm on windows, just FYI. Could you please direct me to a documentation that I could follow step by step and test this out locally from my fork?
🙏

@Joffcom
Copy link
Member

Joffcom commented Feb 21, 2023

Hey @alishaz-polymath,

We have the contributing guide which might help. What issues did you see?

@alishaz-polymath
Copy link
Contributor Author

Hey @alishaz-polymath,

We have the contributing guide which might help. What issues did you see?

This helped. Thanks. 🙏

I could test locally and it did appear to have provided the option. 👍
i would love it if you can test and confirm, and then we can get this resolved. Do I also need to provide necessary update to the docs?

@Joffcom
Copy link
Member

Joffcom commented Feb 23, 2023

Hey @alishaz-polymath,

I don't think any doc changes are needed, It looks like I just missed the release for this week so will be aiming for the 218.0 release next week.

Will get that testing done tomorrow.

@alishaz-polymath
Copy link
Contributor Author

Hey @alishaz-polymath,

I don't think any doc changes are needed, It looks like I just missed the release for this week so will be aiming for the 218.0 release next week.

Will get that testing done tomorrow.

Hey @Joffcom any update on how the test went, and when can we expect this to be merged?
Thanks in advance |🙏

@wjd3
Copy link

wjd3 commented Mar 4, 2023

Is there anything I can help with to get this PR merged? I was going to create my own for this issue anyway.

I'm able to use Cal.com webhooks in the meantime, but this integration is necessary for non-technical users.

@Joffcom
Copy link
Member

Joffcom commented Mar 4, 2023

Hey @alishaz-polymath,

It fell down my list a little bit but I am planning to get this into the next release.

@Joffcom Joffcom changed the title fix(Cal Trigger Node): Cal trigger maintenance fix(Cal Trigger Node): Update to support v2 webhooks Mar 8, 2023
@Joffcom
Copy link
Member

Joffcom commented Mar 8, 2023

Hey @alishaz-polymath & @dayvista,

This looks good and I have approved the change, Once the E2E test is finished I will get it merged in and this change will go out in the next release of n8n.

@Joffcom Joffcom merged commit 2889e53 into n8n-io:master Mar 8, 2023
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Mar 8, 2023
MiloradFilipovic added a commit that referenced this pull request Mar 10, 2023
@MiloradFilipovic
Merge branch 'master' into N8N-5677-workflows-store-refactor
8e2a475 
* master: (358 commits)
  refactor: Remove n8n-core dependency in nodes-base (no-changelog) (#5649)
  🚀 Release 0.219.0 (#5659)
  fix(core): Fix trying to pipe a non stream on errors (no-changelog) (#5660)
  ci: Fix e2e tests (no-changelog) (#5658)
  fix(core): Fix issues with LDAP reset and LDAP init (no-changelog) (#5657)
  feat(HTTP Request Node): Move from Binary Buffer to Binary streaming (#5610)
  feat(editor): Only redirect new users to blank canvas (no-changelog) (#5654)
  feat(editor): Do not automatically add manual trigger on node plus (#5644)
  feat(core): Allow using middlewares with decorators on a per-route basis (no-changelog) (#5656)
  refactor(core): Convert more routes to use the decorator pattern (no-changelog) (#5611)
  fix: Fetch credentials on workflows view to include in duplicated workflows (#5532)
  ci: Add PR checklist (#5628)
  feat(Mindee Node): Add support for v4 API (#5559)
  feat(Microsoft SQL Node): Add support for self signed certificates (#5160)
  fix(editor): Only fetch new versions at app launch (#5647)
  fix(core): Use new version of riot-tmpl in workflow package (no-changelog) (#5619)
  feat(core): Refactor and add SAML preferences for service provider instance (#5637)
  docs(Github Trigger Node): Add notice and more meaningful error around permissions (#5551)
  feat(Cal Trigger Node): Update to support v2 webhooks (#5331)
  feat(editor): Redirect users to canvas if they don't have any workflows (#5629)
  ...
@janober
Copy link
Member

janober commented Mar 10, 2023

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Mar 10, 2023
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Mar 10, 2023
@Joffcom Joffcom removed the Upcoming Release Will be part of the upcoming release label Mar 10, 2023
@alishaz-polymath alishaz-polymath mentioned this pull request Mar 10, 2023
Closed
sunilrr pushed a commit to fl-g6/qp-n8n that referenced this pull request Apr 24, 2023
@alishaz-polymath @sunilrr
feat(Cal Trigger Node): Update to support v2 webhooks (n8n-io#5331)
422eef9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Joffcom Joffcom Joffcom approved these changes

Assignees
No one assigned
Labels
community Authored by a community member node/improvement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alishaz-polymath @Joffcom @wjd3 @janober