fix(core): fix watch task logic for sourceModules #1756
Conversation
garden-service/src/tasks/helpers.ts
Outdated
| }) | ||
| ) | ||
|
|
||
| const serviceNamesForModule = [...module.serviceNames, ...dependantSourceModuleServiceNames] |
There was a problem hiding this comment.
Is this correct? Should we count the services from a source module along with the services for this module?
There was a problem hiding this comment.
The context here is "services for which module has the sources" (which are module's services, and services that have module as their sourceModule). Or am I missing something?
There was a problem hiding this comment.
Maybe the variable name should be serviceNamesUsingModule?
There was a problem hiding this comment.
Ah I see, I had this backwards. Yeah I guess the name is a little confusing, but I think this just needs a comment to explain what's going on.
There was a problem hiding this comment.
Ok, cool. I've renamed the variable—the long comment above was supposed to explain the rationale here, do you think this still needs additional clarification?
There was a problem hiding this comment.
No this is good. Approved and merged!
Before this fix, if a service with a sourceModule was deployed with hot reloading enabled, a build task for its source module would be added when its sources change (in addition to the hot reload task). This was not the desired behavior, and would (e.g. for helm modules using a container module as a sourceModule) result in unnecessary rebuilds of the source module.
thsig
force-pushed
the
sourcemodule-hot-reload-fix
branch
from
7958d12 to
ac34f2c
Compare
What this PR does / why we need it:
Before this fix, if a service with a
sourceModulewas deployed with hot reloading enabled, a build task for its source module would be added when its sources change (in addition to the hot reload task).This was not the desired behavior, and would (e.g. for
helmmodules using acontainermodule as asourceModule) result in unnecessary rebuilds of the source module.Which issue(s) this PR fixes:
This issue was reported by a user on our community Slack.