search

codeamp / panel

CodeAmp Web Application. Built with React.js, GraphQL and Material-UI.
12 stars 7 forks source link

Add cacheable property to extensions and fix Extension layout #274

Closed drshrey closed 6 years ago

drshrey commented 6 years ago

screen shot 2018-10-22 at 1 45 41 pm

iamsaso commented 6 years ago

screenshot pls

drshrey commented 6 years ago

@sasso screenshot added

aballman commented 6 years ago

I don't understand why this is optional. Wouldn't we always want to cache for redeploys? Also, it's very confusing as to how it would be used. Is this only for docker builder? What happens when I enable caching for githubstatus?

aballman commented 6 years ago

https://github.com/codeamp/circuit/issues/346 Indicates that the issue is with workflow extensions. If that's the case why not just always apply this caching logic to workflow extensions and remove this config param?

drshrey commented 6 years ago

@aballman We don't always want to cache and githubstatus is a good example of why. If somebody fails a test but has a built docker image, we'd like to pull again from githubstatus to get the new build statuses without waiting for a docker image to build again. You're right though, caching should only be relevant for workflow types, so I can conditionally show that option depending on the Extension type in that view.

aballman commented 6 years ago

Okay, but what you're telling me is that I can still (because its also a workflow extension) go in and check that box for github status, which would create the situation you are talking about. Whether that's accidental or a bad db query or whatever.

We need to add some error handling to this PR. If there is an error with the admin/extensions page, it just drops you to a white screen with no error message.

drshrey commented 6 years ago

@aballman yup, correct. how did you produce the error?

aballman commented 6 years ago

I still think this is not great. We're solving an issue for a single plugin that becomes systemic. I think we should let the individual plugin decide if it wants to cache or not, not just give all plugins a cacheable property. What if the plugin doesn't use it? You're certainly not forced to implement it or not. Im imagining a debug session where things aren't behaving the way they are expected.

drshrey commented 6 years ago

@aballman Right, I do see what you're saying, but that would involve a much deeper change in which we consume schema definitions from plugins themselves. We should definitely keep tabs to refactor this in the future to a cleaner solution, but from a practical view, we'll be guaranteed to have much faster deploys during rollbacks and redeploys if we merge this. Do you have a situation in mind where this could introduce significant complexity?