search

gluster / anthill

A Kubernetes/OpenShift operator to manage Gluster clusters
http://gluster-anthill.readthedocs.io/
Apache License 2.0
35 stars 12 forks source link

Fixed Typo (%s/Cred/Credentials/) #55

Closed umangachapagain closed 5 years ago

umangachapagain commented 5 years ago

Signed-off-by: Umanga Chapagain chapagainumanga@gmail.com

Describe what this PR does Fixed a typo that was missed out.

Is there anything that requires special attention? None.

Related issues: https://github.com/gluster/anthill/pull/42#discussion_r239804816

humblec commented 5 years ago

@umangachapagain first of all I see this PR need more work than a Typo correction!!! :). Other way, I dont see the field name as a typo, rather its a convention. I dont see a reason or justification that to change the field value, as the json tag is well explained and the field name convey the intention.

umangachapagain commented 5 years ago

@umangachapagain first of all I see this PR need more work than a Typo!!! That said, I dont see a reason or justification that , its a typo and the reason to change the field value as the json tag is well explained.

I was just going through the code and the comments on gluster/anthill/pull/42 where it has been asked to change Cred to Credentials. This had been addressed but looked like this one place was missed out and so I corrected it. Only after this I realized that I need to also regenerate a file and send it as part of this fix plus run gofmt.

If not required, this can be closed or I can fix the rest and update.

P.s. Just getting started with operators

humblec commented 5 years ago

@umangachapagain dont take me wrong. :).. Attempts and PRs are always welcome. On technical side of things, Creds as field name could be enough. But, we could change it as well.

humblec commented 5 years ago

@JohnStrunk any thoughts here ?

JohnStrunk commented 5 years ago

I didn't see the original diff, so I can't comment on that. As it stands currently, the 1-liner is clearly cleanup of a missed change from #42, so :+1:

Looking at the CI failure (and meshing w/ the conversation above), we do need gofmt and operator-sdk generate to be run as a part of this PR. I believe that's the source of the mentioned big diff.

pkg/apis/operator/v1alpha1/glusternode_types.go:1::warning: file is not gofmted with -s (gofmt)
The command "set -o pipefail && gometalinter -j4 --sort path --sort line --sort column --deadline=24h --enable="gofmt" --vendor --debug --exclude="zz_generated.deepcopy.go" ./... |& stdbuf -oL grep "linter took\|:warning:\|:error:"
" exited with 1.

$ ./build.sh "${CONTAINER_REPO}"
# github.com/gluster/anthill/pkg/apis/operator/v1alpha1
pkg/apis/operator/v1alpha1/zz_generated.deepcopy.go:225:7: in.Creds undefined (type *GlusterNodeExternal has no field or method Creds)
pkg/apis/operator/v1alpha1/zz_generated.deepcopy.go:226:17: in.Creds undefined (type *GlusterNodeExternal has no field or method Creds)
pkg/apis/operator/v1alpha1/zz_generated.deepcopy.go:226:29: out.Creds undefined (type *GlusterNodeExternal has no field or method Creds)
FATA[0008] failed to build operator binary: (exit status 2) 
The command "./build.sh "${CONTAINER_REPO}"" exited with 1.

@umangachapagain Please go ahead and take care of these items to clear up CI. Thanks for taking the time to clean up the code.

umangachapagain commented 5 years ago

@humblec @JohnStrunk Done with the cleanup. Please verify.

humblec commented 5 years ago

Thanks! /lgtm. Merging.