-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(base): allow default reconcile behavior to be configured #349
feat(base): allow default reconcile behavior to be configured #349
Conversation
9e328bc
to
571f8ed
Compare
lib/BaseController.js
Outdated
); | ||
} | ||
|
||
reconcileByDefault = String(params.reconcileByDefault); |
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.
interested in your thoughts.. do you think it beneficial to move this reconcileByDefault
into some kind of options
object (ie. params.options.reconcileByDefault
)? My thinking being this leaves open the posibility to add more environment variable configuration in the future if necessary without shoving them all in the params layer, since the params layer has to be updated inside EventHandler. Just want to avoid having to modify 3 files (basecontroller, EventHandler, and the desired controller's index.js like MTP) in the future if possible.
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.
sure, can do
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.
@alewitt2 updated. Is this more of what you were thinking?
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! hopefully will make our lives easier in the future. thanks
571f8ed
to
ff00f47
Compare
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.
lgtm
looks like we need more travis credits.. waiting on response from support then we will merge this one |
@alewitt2 any word? |
nothing yet |
@carrolp any word from support on credits? |
ff00f47
to
9956af8
Compare
@esatterwhite can you update your branch from master |
Base cnntroller accepts a `reconcileByDefault` option which controls the default value for the reconciler behavior when `deploy.razee.io/Reconcile` is not specified. By default it is true. It must be a boolean looking value. 1, or 0 are not valid
9956af8
to
de1a5c1
Compare
Done |
Ping me when this is published. I'll update the controllers |
@esatterwhite its published as 1.3.0 |
Base controller accepts a
reconcileByDefault
option which controls the default value for the reconciler behavior whendeploy.razee.io/Reconcile
is not specified. By default it is true.It must be a boolean looking value. 1, or 0 are not valid