Conversation
diego1q2w
left a comment
diego1q2w
left a comment
There was a problem hiding this comment.
Nice work, but there is a lot of repetitive code and global variables, some functions are to big and hard to read. In future that will make it harder to maintain and for anyone else to implement changes.
Left few comments that may help you to strive to that direction, let me know if there is any questions :D
| if accessKey != "" { | ||
| _accessKey = accessKey | ||
| } |
There was a problem hiding this comment.
to remove all those global variables you can do something like:
| if accessKey != "" { | |
| _accessKey = accessKey | |
| } | |
| if accessKey != "" { | |
| accessKey = os.Getenv("AWS_ACCESS_KEY_ID") | |
| } |
the same with other vars.
There was a problem hiding this comment.
ok, this could be remove, since it is not being used.
| var accessKey string | ||
| var secretAccessKey string | ||
| var sessionToken string |
There was a problem hiding this comment.
is not a good practice to have global variables since someone may mistakenly overwritten them, try to allocate them within a function, and if they are used across many functions is a good idea to use structs
There was a problem hiding this comment.
ok, this could be remove, since it is not being used.
| if serverMtls != "" { | ||
| mtlsCertificate, errorReadFile := os.ReadFile(serverMtls) | ||
| if errorReadFile != nil { | ||
| log.Println(errorReadFile.Error()) |
There was a problem hiding this comment.
you can use a log.Fatal method, that will terminate the program with a log, so the user can fix the issue
| log.Println(errorReadFile.Error()) | |
| log.Fatal(errorReadFile.Error()) |
There was a problem hiding this comment.
I would like program to could continue to run. I will not change to log.Fatal then.
| if serverCertificate != "" && serverPrivateKey != "" { | ||
| var config *tls.Config | ||
| if serverMtls != "" { | ||
| mtlsCertificate, errorReadFile := os.ReadFile(serverMtls) |
There was a problem hiding this comment.
why we need the certificate?
There was a problem hiding this comment.
mtlsCertificate, it is for mTLS(mutual tls) configuration. however it is optional
serverCertificate and serverPrivateKey, it is for https. however it is optional
| if errorDescribeInstances == nil { | ||
| for _, reservation := range _describeInstancesOutput.Reservations { | ||
| for _, instance := range reservation.Instances { | ||
| _instanceId := *instance.InstanceId | ||
| _publicIpAddress := "" | ||
| _privateIpAddress := "" | ||
| _tagName := "" | ||
| if instance.PublicIpAddress != nil { | ||
| _publicIpAddress = *instance.PublicIpAddress | ||
| } | ||
| if instance.PrivateIpAddress != nil { | ||
| _privateIpAddress = *instance.PrivateIpAddress | ||
| } | ||
| for _, tag := range instance.Tags { | ||
| if tag.Key != nil && *tag.Key == "Name" && tag.Value != nil { | ||
| _tagName = *tag.Value | ||
| } | ||
| } | ||
| _ec2s = append(_ec2s, common.Ec2Dto{ | ||
| TagName: _tagName, | ||
| InstanceId: _instanceId, | ||
| PublicIP: _publicIpAddress, | ||
| PrivateIP: _privateIpAddress, | ||
| Region: *region.RegionName, | ||
| }) | ||
| } | ||
| } | ||
| } else { | ||
| println(errorDescribeInstances.Error()) | ||
| } |
There was a problem hiding this comment.
all this can go to it's own method, it'll be easier to read and maintatin in the future
| var _accessToken string | ||
|
|
||
| func main() { | ||
| flag.StringVar(&serverCertificate, "server-certificate", "", "") |
There was a problem hiding this comment.
I see a lot of repetetive code across different cloud providers, this is a good example to introduce clean architecure concepts.
I work as follows
cmd/cmd is just for calling methods from pkg, the main file should be no longer than a few lines since is just calling methods- aws/main
- do/main
- hetzner/main
pkg/here you put all the bussines logic, that way you can reuse componentsnodedb/domain or app nameinfra/folder where you run all the requests to the cloud providersadapter/folder where you build the api or access point (in this case the cli)logic filesthis is where you put all the bussines logic this is not a folder but a bunch of files, this is where you would put all the common structs
| @@ -0,0 +1,7 @@ | |||
| package common | |||
|
|
|||
| type ConfigurationDto struct { | |||
There was a problem hiding this comment.
all this structs could go inside the pkg/nodedb/dto.go file
| os.Exit(0) | ||
| } | ||
|
|
||
| func Home(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
could you move this to the pkg directory?
2 participants
No description provided.