-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Auto Sync and Re-Sync for Manually Created Integrations #6071
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, I think we can do a bit more to make this flow nice, by showing the user the webhook URL & secret always.
integration is not functioning correctly, try resyncing the webhook: | ||
This webhook was configured when this project was imported | ||
or it was manually created with correct configuration. If this | ||
integration is not functioning correctly, try re-syncing the webhook: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I'd like to see us always displaying the webhook URL information, so that users can confirm that the automated webhook is the one they see in their GitHub admin. We should always show this data.
BTW, this is so much better than making users login to GitHub and create the webhook manually. This is a huge win! |
@ericholscher ready for a full review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good with one small change 👍
user_pk=self.request.user.pk, | ||
integration=self.object | ||
) | ||
if self.object.integration_type != Integration.API_WEBHOOK: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not have some kind of has_sync
check here? I feel like this might break in the future as we add new types of Integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think what you said would be better. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericholscher Updated the PR!
A lot of work needs to be done and a lot of cases needs to be handled before its ready for review. :)
closes #3932
related to the first step of #6052