-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[server] Don't fetch the repository config a second time when starting incremental prebuilds #13962
Conversation
started the job as gitpod-build-jx-fetch-config.1 because the annotations in the pull request description changed |
Tested incremental prebuilds and they still work 🎉 |
config?: WorkspaceConfig, | ||
): Promise<PrebuiltWorkspace | undefined> { | ||
if (!history.commitHistory || history.commitHistory.length < 1) { | ||
return; | ||
} | ||
|
||
const { config } = await this.configProvider.fetchConfig({}, user, context); | ||
if (!config) { | ||
config = (await this.configProvider.fetchConfig({}, user, context)).config; | ||
} |
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.
We're always gonna fetch the config when it's not provided. Here, we can push that into caller and always require a config to be present on this call. The caller can do await this.configProvider.fetchConfig({}, user, context)).config
to get it. It has the benefit of not hiding complexity and the method signature much better communicates the behaviour.
We only have another caller from gitpod-server-impl
so it is a small change.
config: WorkspaceConfig,
): Promise<PrebuiltWorkspace | undefined> {
if (!history.commitHistory || history.commitHistory.length < 1) {
return;
}
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.
Good point, thanks for the idea 👍
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 implies injecting a ConfigProvider
into GitpodServerImplEE
, but I think that's okay and cleaner this way.
/hold for Milan's suggestion |
…g incremental prebuilds
1a183fc
to
e82d33e
Compare
Addressed /unhold |
Description
Follow-up to #13801: Don't fetch the repository config a second time when starting incremental prebuilds
Related Issue(s)
Fixes #13801 (comment)
How to test
Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide