Closed JohnStrunk closed 5 years ago
@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.
I modified the pre-commit hook to match the chacks in the CSI repo
typo on 'chacks' :), please correct it.
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 *_types.go
and *_controller.go
files are done at our peril. Basically, the SDK generates the other stuff for us, and if we modify it, it will be very difficult to integrate SDK improvements into our code. Basically, for any file we modify we are volunteering to manually merge in SDK fixes and improvements.
As for the "remove me" type comments, yes we should remove them... once we actually put some code in there. :smile:
I believe I have addressed all comments.
BTW, upgrade discussion here: https://github.com/operator-framework/operator-sdk/issues/773
@JohnStrunk I still think USER ACTION REQUIRED
comments can be deleted as it wont look good on a merged PR/code :), but I dont want to insist on this though.
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.
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.