-
Notifications
You must be signed in to change notification settings - Fork 2
[ID-358] App config version control integration #498
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
base: develop
Are you sure you want to change the base?
Conversation
shurwit
left a comment
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 @siqiz7, this is looking good! I left a few more comments below
shurwit
left a comment
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 @siqiz7, this is looking good! Just a couple more comments below
driven/github/adapter.go
Outdated
|
|
||
| // UpdateCachedWebhookConfigs updates the webhook configs cache | ||
| func (sa *Adapter) UpdateCachedWebhookConfigs() error { | ||
| return sa.cacheWebhookConfigs() |
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.
There is currently an issue here when the webhook configs are updated. The cache will be updated on whichever instance receives the webhook request, but the other instances will not get the updated version. The easiest solution would be to store these webhook configs into the database somewhere and setting a listener so that all instances will be notified of updates.
shurwit
left a comment
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 @siqiz7, LGTM! I have restructured this to abstract all GitHub specific implementations into the github package. This is so we can support different VCS systems in the future.
@petyos, please take a look when you get a chance. Also please note that there are several related issues that build on this one that we will request your review on soon as well. Thanks!
petyos
left a comment
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.
LGTM
|
Hi @petyos, @roberlander2 and I noticed that we will need to make a change here to handle creating a new version object in the app type if a new app config is added with a version number that doesn't currently exist. Otherwise, we will end up with conflicting version number IDs between the app types and app configs. We will address this before merging. |
Resolves #358