-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
Signed-off-by: John Strunk <jstrunk@redhat.com>
This adds the results of running: `operator-sdk new anthill --skip-git-init` and merging those files into the anthill repo unchanged. The initial `vendor` directory has not been committed. Signed-off-by: John Strunk <jstrunk@redhat.com>
Signed-off-by: John Strunk <jstrunk@redhat.com>
The operator-sdk produced yamls w/ poor indentation. This fixes the affected files so they pass yamllint Signed-off-by: John Strunk <jstrunk@redhat.com>
Results of running: `operator-sdk add api --api-version=operator.gluster.org/v1alpha1 --kind=GlusterCluster` Signed-off-by: John Strunk <jstrunk@redhat.com>
Signed-off-by: John Strunk <jstrunk@redhat.com>
This is the result of: `operator-sdk add controller --api-version=operator.gluster.org/v1alpha1 --kind=GlusterCluster` Signed-off-by: John Strunk <jstrunk@redhat.com>
Signed-off-by: John Strunk <jstrunk@redhat.com>
Results of running: `operator-sdk add api --api-version=operator.gluster.org/v1alpha1 --kind=GlusterNode` and fixing yaml formatting Signed-off-by: John Strunk <jstrunk@redhat.com>
Results of running: `operator-sdk add controller --api-version=operator.gluster.org/v1alpha1 --kind=GlusterNode` Signed-off-by: John Strunk <jstrunk@redhat.com>
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.
Code! :-)
Some small nits, but overall I'm liking what I'm seeing.
cmd/manager/main.go
Outdated
"runtime" | ||
|
||
"github.com/gluster/anthill/pkg/apis" | ||
"github.com/gluster/anthill/pkg/controller" |
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 understand that this is generated code, but it would be nice (in a followup commit) to order the import sections as:
- std library
- outside the repo/project
- inside the repo/project
This is a standard more projects have been following. @Madhu-1 was just making similar fixes in heketi.
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.
Yeah... Right now, I don't have a feel for what gets overwritten by the sdk and what doesn't. I suspect it's fair game for this file to get overwritten by the sdk.
I think we're supposed to stick to the *_types.go
and *_controller.go
files.
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 believe that except the zz_generated
files, the rest of the code is okay to be changed. zz_generated
files are actually generated from other code. The rest of the code is really just skeleton code that we need to fill up.
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 agree with @kshlm , rest is in our control and we should be able to correct it.
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.
Re-ordered
|
||
Once the SDK is installed, Anthill can be built via: | ||
|
||
```bash |
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 think you should mention you need to run dep ensure
before building using operator-sdk build. If you naively follow the steps here, like I did the first time, it fails to find the dependency packages.
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.
Good point. done.
Signed-off-by: John Strunk <jstrunk@redhat.com>
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! |
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.
Do we need this here ?
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.
@shtripat will remove this when he posts his PR w/ the types.
/** | ||
* USER ACTION REQUIRED: This is a scaffold file intended for the user to modify with their own Controller | ||
* business logic. Delete these comments after modifying this file.* | ||
*/ |
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.
Yes, we need to delete these comments :)
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.
Just remove the commented lines or retain them till we add new logic here?
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 say keep the comments until content is actually added.
/** | ||
* USER ACTION REQUIRED: This is a scaffold file intended for the user to modify with their own Controller | ||
* business logic. Delete these comments after modifying this file.* | ||
*/ |
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.
Delete above line ?
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.
Just remove the commented lines or retain them till we add new logic here?
@JohnStrunk As this is mostly auto generated, its fine to have initial skeleton. however we chould do some source code cleanup and some corrections. Please see my review comments. |
typo on 'chacks' :), please correct it. |
README.md
Outdated
$ operator-sdk build docker.io/gluster/anthill | ||
INFO[0001] Building Docker image docker.io/gluster/anthill | ||
Sending build context to Docker daemon 114.7MB | ||
Step 1/4 : FROM alpine:3.8 |
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.
Do we need to base our dockerimage on alphine ? I think its good to base on CentOS .. thougts are welome.
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.
We should be ok customizing the Dockerfile. This is just what the SDK created. I think we can probably build FROM scratch
, which would be awesome!
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 just checked on FROM scratch
, and that won't work because when you enable testing in the sdk, it injects a shell script into the container. Should be able to move to centos though.
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.
Switched to CentOS
role.rbac.authorization.k8s.io "anthill" created | ||
|
||
$ kubectl apply -f deploy/role_binding.yaml | ||
rolebinding.rbac.authorization.k8s.io "anthill" created |
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.
The roles can be combined in a single Yaml file.
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.
done
Lots of the comments thus far have been related to reorganizing or modifying the autogenerated files. My reading of the SDK documentation suggests that modifications done to files other than As for the "remove me" type comments, yes we should remove them... once we actually put some code in there. 😄 |
) | ||
|
||
func printVersion() { | ||
logf.Log.Info(fmt.Sprintf("Go Version: %s", runtime.Version())) |
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.
do we have logf.Log.Infof
instead of logf.Log.Info
to avoid use of Sprinf
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.
There is no InfoF equivalent. See: https://godoc.org/github.com/go-logr/logr
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 imagine the use of logr is part of the SDK? Looks kinda gross, but that might just be my unfamiliarity with it.
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.
My complaint about the docs were resolved and I don't think there's an immediate need to fix all the formatting, etc issues in the autogenerated files right away I'm good with merging this and then iterating on those files as needed.
Signed-off-by: John Strunk <jstrunk@redhat.com>
Signed-off-by: John Strunk <jstrunk@redhat.com>
Signed-off-by: John Strunk <jstrunk@redhat.com>
I believe I have addressed all comments. |
BTW, upgrade discussion here: operator-framework/operator-sdk#773 |
@JohnStrunk I still think |
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.
A few minor nits. Overall it's almost good enough that this could go through as-is. I'm in favor of leaving the boilerplate comments in so we have guideposts when actual development starts.
One thing I don't understand, why did we switch to a CentOS based VM? We don't require anything specific to the OS, and it results in a larger container image size. I'm personally in favor of sticking with alpine.
) | ||
|
||
func printVersion() { | ||
logf.Log.Info(fmt.Sprintf("Go Version: %s", runtime.Version())) |
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 imagine the use of logr is part of the SDK? Looks kinda gross, but that might just be my unfamiliarity with it.
deploy/role.yaml
Outdated
- '*' | ||
|
||
--- | ||
|
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.
Remove whitespace
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.
fixed
/** | ||
* USER ACTION REQUIRED: This is a scaffold file intended for the user to modify with their own Controller | ||
* business logic. Delete these comments after modifying this file.* | ||
*/ |
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 say keep the comments until content is actually added.
Signed-off-by: John Strunk <jstrunk@redhat.com>
Re CentOS vs. Alpine: CentOS will make an easier transition to the product that will be RHEL-based. Also, CentOS is the base of all the other GCS images, so it should be common on all the nodes. But yes, in isolation, it's bigger. There are docs about changing the logger, but also a warning that doing so will discard log messages from controller runtime. While the syntax is a bit ugly, I'd like to keep the controller runtime logs. Details here. |
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.
Re CentOS vs. Alpine: CentOS will make an easier transition to the product that will be RHEL-based.
Ah, legit.
Also, CentOS is the base of all the other GCS images, so it should be common on all the nodes. But yes, in isolation, it's bigger.
Also legit.
There are docs about changing the logger, but also a warning that doing so will discard log messages from controller runtime. While the syntax is a bit ugly, I'd like to keep the controller runtime logs. Details here.
I figured there was something like that.
LGTM!
LGTM .. @phlogistonjohn @shtripat If no other comments, I will merge this PR . |
Describe what this PR does
This adds an initial operator executable based on the Operator SDK.
Is there anything that requires special attention?
There is a significant amount of code here, however it is almost all auto-generated by the
operator-sdk
tool.Related issues:
Related to #30 - This partially satisfies #30, but it does not have the actual CRD contents defined, so it cannot be considered a fix.