Adds cluster config generation with kubeadm #113
Conversation
prateekgogia
requested review from
ellistarn and
hakuna-matatah
as code owners
| @@ -46,6 +46,9 @@ func (s *Subnets) Create(ctx context.Context, substrate *v1alpha1.Substrate) (re | |||
| return reconcile.Result{}, err | |||
| } | |||
| for _, subnet := range subnets { | |||
| if subnet == nil { // we can run into a case when ctx is canceled, errs and subnets are all nil | |||
There was a problem hiding this comment.
If ctx is cancelled, don't you have an error value caught on line 45?
There was a problem hiding this comment.
No I ran into this, err was nil and because we have subnets slice with nils defined on line 40, we enter this for loop.
There was a problem hiding this comment.
There's gotta be a bug somewhere else.
| ) | ||
|
|
||
| const ( | ||
| clusterCertsBasePath = "/tmp/" |
There was a problem hiding this comment.
Can we use https://pkg.go.dev/io/ioutil#TempDir
There was a problem hiding this comment.
IIUC, this will create a temp dir with random strings as suffix, we want to discover existing certs and configs atleast locally if someone runs the cli again to create same substrate cluster.
There was a problem hiding this comment.
Certs probably shouldn't change. Kubernete configs need to change.
There was a problem hiding this comment.
You're also not guaranteed that the host os even has a /tmp dir, which the go library has mechanisms to handle.
There was a problem hiding this comment.
lets discuss about this, I am probably missing some context here
| } | ||
|
|
||
| // UploadObject uploads a file | ||
| func (d *DirectoryIterator) UploadObject() s3manager.BatchUploadObject { |
There was a problem hiding this comment.
Does this upload in parallel?
There was a problem hiding this comment.
The SDK enables you to pass custom iterators to the new batched operations. For example, if we want to upload a directory, none of the default iterators do this easily. The following example shows how to implement a custom iterator that uploads a directory to S3.
Man -- instead of writing docs, why not write a directory iterator :O
| } | ||
|
|
||
| // Err returns error of DirectoryIterator | ||
| func (d *DirectoryIterator) Err() error { |
There was a problem hiding this comment.
I see you're just copying the docs, but this impl is pretty weak. We can leave as is for now.
There was a problem hiding this comment.
yeah I wanted something quick and that worked. Later I found I also used this same logic in the earlier implementation Lol.
We can revisit and make it more robust.
There was a problem hiding this comment.
Can you cut an issue? label: operational-excellence
|
|
||
| func (c *Config) Delete(ctx context.Context, substrate *v1alpha1.Substrate) (reconcile.Result, error) { | ||
| // delete the s3 bucket | ||
| if err := s3manager.NewBatchDeleteWithClient(c.S3).Delete(ctx, s3manager.NewDeleteListIterator( |
There was a problem hiding this comment.
I don't think you need to delete the files. Just delete the bucket.
There was a problem hiding this comment.
It fails to delete the bucket if it still has files in it, I will check if there is a --force kind of option
There was a problem hiding this comment.
Looked at this there is no force option in the request
prateekgogia
changed the title
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.