-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Adding support for Kubernetes to local CRE #20519
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
|
I see you updated files related to
|
19717f4 to
f2a509a
Compare
f2a509a to
2dd7429
Compare
|
|
| func Start( | ||
| ctx context.Context, | ||
| testLogger zerolog.Logger, | ||
| commonLogger logger.Logger, | ||
| inputs []*blockchain.Input, | ||
| deployers map[blockchain.ChainFamily]Deployer, | ||
| provider *infra.Provider, |
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.
why do we need it? deployers already have access to provider. Check here: https://github.com/smartcontractkit/chainlink/blob/develop/system-tests/lib/cre/environment/blockchains/evm/evm.go#L45
| @@ -56,6 +56,7 @@ func StartDONs( | |||
| capabilityConfigs cre.CapabilityConfigs, | |||
| copyCapabilityBinaries bool, | |||
| nodeSets []*cre.NodeSet, | |||
| apiUser, apiPassword string, | |||
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.
I'd rather have a API user/password provider that would return this or that value per each infra type and inject that here than have user/password fields that are only used in case of k8s. It will be confusing for users.
| @@ -70,31 +71,48 @@ func StartDONs( | |||
| if devspaceErr != nil { | |||
| return nil, pkgerrors.Wrap(devspaceErr, "failed to deploy Dons with crib-sdk") | |||
| } | |||
| } | |||
| } else if infraInput.IsKubernetes() { | |||
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 we use switch instead of ifs? here and in all the other places that care about infra type 🙏
| if flags.HasFlagForAnyChain(donMetadata.Flags, flag) && config.BinaryPath != "" { | ||
| customBinariesPaths[flag] = config.BinaryPath | ||
| // Skip binary operations for Kubernetes (binaries are in the cluster images) | ||
| if !infraInput.IsKubernetes() { |
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.
since this copying has to be done only for Docker why not do this instead:
if infra.IsDocker() {
// copy code goes here
}
| infra.PrintFailedContainerLogs(lggr, 30) | ||
| if !infraInput.IsKubernetes() { | ||
| infra.PrintFailedContainerLogs(lggr, 30) | ||
| } |
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.
if infraInput.IsDocker() {
infra.PrintFailedContainerLogs(lggr, 30)
}
| } | ||
| if apiPassword == "" { | ||
| apiPassword = "password" // Required default for testing | ||
| } |
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.
as mentioned before, this should all be encapsulated in a credentials provider
| // as we don't have a way to get its database connection string | ||
| if input.Provider.Type == infra.CRIB { | ||
| if input.Provider.Type == infra.CRIB || input.Provider.IsKubernetes() { |
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.
if !input.Provider.IsDocker() {
return nil
}
| func GenerateKubernetesJDOutput(infraInput *Provider, lggr zerolog.Logger) *jd.Output { | ||
| externalDomain := "" | ||
|
|
||
| if infraInput.Kubernetes != nil { |
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.
why not just return err if infraInput.Kubernetes is nil and never check it again?
| func GenerateKubernetesNodeSetOutput(infraInput *Provider, nodeSetName string, nodeCount int, nodeMetadataRoles []bool, apiUser, apiPassword string, lggr zerolog.Logger) *ns.Output { | ||
| externalDomain := "" | ||
|
|
||
| if infraInput.Kubernetes != nil { |
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, check early and return err if it is nil




What
Adding support for Kubernetes to local CRE.
Example infra configuration:
Requires
N/A
Supports
N/A