Skip to content

Conversation

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Aug 7, 2018

  • lib/resourceread,resourcemerge,resourceapply are sourced from https://github.com/openshift/library-go/tree/master/pkg/operator/resource and changed for local use.
  • manifest/ has managed manifests. (these are stored as generated code in pkg/operator/assets/bindata.go)
  • install/ has assets that install operator. (these are stored as generated code in pkg/operator/assets/bindata.go)
  • pkg/operator adds inits sync loop for MCO
  • cmd/machine-config-operator adds the command machine-config-operator
  • add 2-stage dockerfiles for machine-config-operator,controller

Follow up:

  • Bootstrap phase for MCO
  • tests for MCO
  • Remove hard-coded controllerconfig when installconfig is available

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 7, 2018
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use WORKDIR instead of changing directory.

@abhinavdahiya abhinavdahiya force-pushed the operator branch 4 times, most recently from 5741373 to 59713cd Compare August 9, 2018 00:45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you use a for loop for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, we can create a map from the asset name to their render and read functions, and potentially wait for the resources. Should be pretty generic to reduce the code redundancy .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read* and apply* expect specific types.
Will end up doing bunch of type assertions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

@yifan-gu
Copy link

yifan-gu commented Aug 9, 2018

@abhinavdahiya LGTM overall, but seems a lot of the reconcile code can be compressed

@abhinavdahiya abhinavdahiya force-pushed the operator branch 2 times, most recently from 2009ce6 to d7d0cc7 Compare August 9, 2018 22:37
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2018
@abhinavdahiya
Copy link
Contributor Author

@yifan-gu PTAL

Copy link

@yifan-gu yifan-gu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018
@abhinavdahiya abhinavdahiya merged commit a0ac334 into openshift:master Aug 13, 2018
@abhinavdahiya abhinavdahiya deleted the operator branch August 13, 2018 19:57
alaypatel07 pushed a commit to alaypatel07/machine-config-operator that referenced this pull request Oct 30, 2019
*: add render keys to generate appropriate commands for etcd certs
osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021
deploy: add ordering to manifests that end up in updatepayload
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants