-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(k8s): in-cluster building #808
Conversation
fd27897
to
88f5b8e
Compare
Note: This needs a little more testing before merging. I think I see some merge-related issues that the compiler isn't catching. |
Ok, resolved the issues I could find. But it'd be great to get more hands on this, to test and make sure I didn't mess anything else up in the merge. |
aaf0257
to
c4df7c2
Compare
Commands always fail with:
Even though I've already run This issue seems to have been introduced in #803. |
Also, {
'docker-daemon':
{
state: 'outdated',
version: undefined,
detail: { remoteObjects: [Array] }
},
'docker-registry':
{
state: 'outdated',
version: undefined,
detail: { remoteObjects: [Array] }
},
'registry-proxy':
{
state: 'ready',
version: 'v-cc8c98d873',
detail: { remoteObjects: [Array] }
}
} which results in Garden initialising the env on every command. EDIT: This is after I comment out the line which always sets |
garden-service/src/actions.ts
Outdated
@@ -158,8 +158,8 @@ export class ActionHelper implements TypeGuard { | |||
if (!allowUserInput && needManualInit.length > 0) { | |||
const names = needManualInit.map(s => s.name).join(", ") | |||
const msgPrefix = needManualInit.length === 1 | |||
? `Plugin ${names} has been updated or hasn't been configured, and requires user input.` | |||
: `Plugins ${names} have been updated or haven't been configured, and require user input.` | |||
? `Provider ${names} has been updated or hasn't been configured, and requires manual initialization.` |
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's a dot at the end of the prefix and where the message is printed, resulting in two dots.
garden-service/src/actions.ts
Outdated
? `Plugin ${names} has been updated or hasn't been configured, and requires user input.` | ||
: `Plugins ${names} have been updated or haven't been configured, and require user input.` | ||
? `Provider ${names} has been updated or hasn't been configured, and requires manual initialization.` | ||
: `Providers ${names} have been updated or haven't been configured, and require manual initialization.` |
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.
Same.
There's still the issue with
|
Ah, I misunderstood the flow, I thought that would only kick in if the environment wasn't reported as ready. I'll fix that. |
), | ||
}) | ||
} else if (systemServiceStatuses.state !== "ready") { | ||
systemReady = false |
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.
Could this not be accounted for where systemReady
is set above? Would be more readable imo.
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.
The statement would then look like this:
systemReady = systemTillerReady && sysNamespaceUpToDate && (
systemServiceStatuses.state === "ready"
||
(needManualInit && systemServiceStatuses.state === "outdated")
)
Guess that's in the eye of the beholder, I'm happy to go with either.
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.
It's not the prettiest of assignments but IMO the control flow is slightly more readable this way. But I can swing either way of course.
LGTM. Did some very basic tests but still working on running the whole thing through our integ suite, got stuck on a tangent earlier today. |
Moving this from our closed-source version. This adds an option to build container images inside Kubernetes clusters, and by extension removes the need for Docker and Kubernetes running on developer machines. Use it by setting `buildMode: "cluster-docker"` in the `kubernetes` provider configuration.
…iffs Also improved the display of diffs when verbose logging, to make these types of issues a little easier to debug.
This is a temporary fix before we manage to fully normalize manifests while diffing, i.e. when server-side apply is widely available for all/most resources.
Moving this from our closed-source version. This adds an option to build
container images inside Kubernetes clusters, and by extension removes the
need for Docker and Kubernetes running on developer machines.
Use it by setting
buildMode: "cluster-docker"
in thekubernetes
providerconfiguration.