Skip to content
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

handle optionals during drift detection and updates for namespace #12

Merged
merged 7 commits into from
Apr 16, 2020

Conversation

lanwen
Copy link
Contributor

@lanwen lanwen commented Apr 15, 2020

Since pulsar have some defaults, it's quite beneficial to allow
undefined values in the tf definition.
That basically means "not managed by tf". But in the previous state
of this provider null-values were submitted in any case,
sometimes silently having breaking a valid setup or just facing a validation on
the pulsar side and failing an update.

This PR brings per-prop check, to avoid unnecessary api calls and be more precise on what is really updating.
That way if any of the optionals left empty, it wouldn't be changed on the pulsar side.

To mitigate some tf limitations, where tf doesn't know if props of an element of the set defined,
this PR brings some validation and assuming defaults which are invalid,
to simply ignore it later on update/create.

This leads to side effect of never empty plan after update when only part of the namespace_config specified.
image

That should encourage user to define all the properties. Another option would be to flatten this config and
use tf capabilities to check if its defined, but that would require migration of the state and config.

fixes #11

Since pulsar have some defaults, it's quite beneficial to allow
undefined values in the tf definition.
That basically means "not managed by tf". But in the previous state
of this provider null-values were submitted in any case,
sometimes silently breaking a valid setup or just facing a validation on
the pulsar side and failing an update.

This PR brings per-prop check, to avoid unnecessary api calls and be more precise on what is really updating.
That way if any of the optionals left empty, it wouldn't be changed on the pulsar side.

To mitigate some tf limitations, where tf doesn't know if props of an element of the set defined,
this PR brings some validation and assuming defaults which are invalid,
to simply ignore it later on update/create.

This leads to side effect of never empty plan after update when only part of the `namespace_config` specified.
That should encourage user to define all the properties. Another option would be to flatten this config and
use tf capabilities to check if its defined, but that would require migration of the state and config.
@lanwen lanwen marked this pull request as ready for review April 15, 2020 11:05
@lanwen lanwen mentioned this pull request Apr 15, 2020
@sijie sijie added this to the 0.1.0 milestone Apr 16, 2020
@sijie
Copy link
Member

sijie commented Apr 16, 2020

@lanwen awesome contributions! +1

@sijie sijie merged commit 1de7520 into streamnative:master Apr 16, 2020
@lanwen lanwen deleted the handle branch April 16, 2020 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: ERROR_CREATE_NAMESPACE_CONFIG: 2 errors occurred: SetNamespaceReplicationClusters code: 500
2 participants