chore: update comments and remove unnecessary conversions and checks #21
Conversation
|
@hezhizhen ok, i will review it. |
api/v1/curvecluster_types.go
Outdated
| ClusterPhasePending ConditionType = "Pending" | ||
| // ClusterReady | ||
| // ClusterPhaseReady : The cluster has been created successfully. |
There was a problem hiding this comment.
you would remove the colon will be more standardized
There was a problem hiding this comment.
I'll polish these comments later.
api/v1/curvecluster_types.go
Outdated
| ClusterPhaseUnknown ConditionType = "Unknown" | ||
| ) | ||
|
|
||
| const ( | ||
| // ConditionTypeEtcdReady | ||
| // ConditionTypeEtcdReady ... |
There was a problem hiding this comment.
Add some description here.
api/v1/curvecluster_types.go
Outdated
| type S3ConfigSpec struct { | ||
| AK string `json:"ak,omitempty"` | ||
| SK string `json:"sk,omitempty"` | ||
| NosAddress string `json:"nosAddress,omitempty"` | ||
| SnapShotBucketName string `json:"bucketName,omitempty"` | ||
| } | ||
|
|
||
| // StorageScopeSpec | ||
| // StorageScopeSpec ... |
There was a problem hiding this comment.
So, maybe you can add more description for EtcdSpec, MdsSpec, SnapShotCloneSpec, S3ConfigSpec.
pkg/chunkserver/chunkfilepool.go
Outdated
| @@ -17,8 +17,8 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| PrepareJobName = "prepare-chunkfile" | |||
| DEFAULT_CHUNKFILE_SIZE = 16 * 1024 * 1024 // 16MB | |||
There was a problem hiding this comment.
For numbers, uppercase should be better, but you can modify the initials of 'PrepareJobName' and other variables in another package , because other packages do not use this value.
There was a problem hiding this comment.
It's not Go's convention, but it's okay to make it a convention for this repository.
There was a problem hiding this comment.
PrepareJobName is used in controllers, so I can't change it to be unexported.
pkg/chunkserver/spec.go
Outdated
| @@ -358,7 +358,7 @@ func (c *Cluster) makeCSDaemonContainer(csConfig *chunkserverConfig) v1.Containe | |||
| return container | |||
| } | |||
|
|
|||
| // getChunkServerPodLabels | |||
| // getChunkServerPodLabels ... | |||
There was a problem hiding this comment.
some more description?
pkg/chunkserver/volume.go
Outdated
| @@ -185,7 +185,7 @@ func CSConfigConfigMapVolumeAndMount(csConfig *chunkserverConfig) ([]v1.Volume, | |||
| return vols, mounts | |||
| } | |||
|
|
|||
| // createTopoAndToolVolumeAndMount | |||
| // createTopoAndToolVolumeAndMount ... | |||
There was a problem hiding this comment.
some more description?
pkg/config/variables.go
Outdated
| @@ -193,9 +193,8 @@ func getValue(name string, dc ConfigInterface) string { | |||
| return "" | |||
| } | |||
|
|
|||
| // ReplaceConfigVars | |||
| // ReplaceConfigVars ... | |||
There was a problem hiding this comment.
some more description?
| if _, ok := r.ClusterController.clusterMap[curveCluster.Namespace]; ok { | ||
| delete(r.ClusterController.clusterMap, curveCluster.Namespace) | ||
| } | ||
| delete(r.ClusterController.clusterMap, curveCluster.Namespace) |
There was a problem hiding this comment.
You're right, but it's more more intuitive using if.
There was a problem hiding this comment.
deletion is idempotent, so it deletes curveCluster.Namespace from the map if the key exists, and does nothing if not.
With if, you are emphasizing it; but it's not necessary actually because it's guaranteed by Go's compiler.
There was a problem hiding this comment.
yes, but it may be more obvious to developers and me.
There was a problem hiding this comment.
yes, but it may be more obvious to developers and me.
Okay, I'll revert it.
pkg/daemon/volume.go
Outdated
| @@ -26,7 +26,7 @@ func DaemonVolumes(configMapDataKey string, configMapMountPathDir string, dataPa | |||
| return vols | |||
| } | |||
|
|
|||
| // DaemonVolumeMounts returns the pod container volume mounth used by Curve daemon | |||
| // DaemonVolumeMounts returns the pod container volume mounts used by Curve daemon | |||
There was a problem hiding this comment.
volumeMounts my be better?
pkg/k8sutil/condition.go
Outdated
|
|
||
| return nil | ||
| return errors.Wrapf(err, "failed to update object %q status", nsName.String()) |
hezhizhen
force-pushed
the
chore
branch
5 times, most recently
from
26ee49f to
25ec5bc
Compare
|
lgtm |
Some cleanup
@caoxianfei1