refactor(provider/oracle, storage/oracle): oraclebmcs provider/storage is renamed to oracle #959
Conversation
| @@ -250,15 +250,15 @@ | |||
| * [**hal config provider openstack bakery edit**](#hal-config-provider-openstack-bakery-edit) | |||
| * [**hal config provider openstack disable**](#hal-config-provider-openstack-disable) | |||
| * [**hal config provider openstack enable**](#hal-config-provider-openstack-enable) | |||
| * [**hal config provider oraclebmcs**](#hal-config-provider-oraclebmcs) | |||
There was a problem hiding this comment.
I'm worried this kind of renaming will break any existing users... is this necessary?
There was a problem hiding this comment.
It is not strictly necessary, but it is highly desirable from our point of view. The original name was probably an unfortunate choice, and was tied to a name that is no longer used. So leaving it as it is could also cause possible confusion. I would like to suffer any pain now while it is hopefully a smaller impact of changing this.
There was a problem hiding this comment.
If this change would force people who do not even use that provider to take some action, like update their hal config file, then we would definitely not want to do that.
sync fork markxnelson
guoyongzhang
force-pushed
the
object-store-support
branch
from
0daf582
to
d3277d4
Compare
guoyongzhang
changed the title
…ider/storage from oraclebmcs to oracle
guoyongzhang
force-pushed
the
object-store-support
branch
from
d3277d4
to
e3ee328
Compare
|
The provider/storage config will read both old name (oraclebmcs) and new name (oracle), and consolidate them under just new name (oracle) for hal config to display and hal deploy to generate deployment files. With this PR, spinnaker with old oraclebmcs provider/storage config should be ported to the new name "oracle" automatically. |
guoyongzhang
changed the title
| @Override | ||
| public NodeIterator getChildren() { | ||
| return NodeIteratorFactory.makeReflectiveIterator(this); | ||
| List<Node> nodes = new ArrayList<Node>(); |
There was a problem hiding this comment.
Can we keep using reflective iterator, but maybe extend it to take preconditions? e.g. name != oraclebmcs. Otherwise this just adds to the overhead for adding a new provider.
| nodes.add(gcs); | ||
| nodes.add(redis); | ||
| nodes.add(s3); | ||
| nodes.add(OraclePersistentStore.mergeOracleBMCSPersistentStore(oracle, oraclebmcs)); |
There was a problem hiding this comment.
Same comment here about relying on the reflective iterator
|
changes made according to the suggestion. |
|
Please review. This PR contains multiple commits, should I squash them or the merge can do it? |
|
LGTM |
| @@ -46,16 +51,35 @@ | |||
| GoogleProvider google = new GoogleProvider(); | |||
| KubernetesProvider kubernetes = new KubernetesProvider(); | |||
| OpenstackProvider openstack = new OpenstackProvider(); | |||
| @JsonProperty(access = Access.WRITE_ONLY) | |||
There was a problem hiding this comment.
If this is write only, and someone has the written in their halconfig, how does halyard read it to do the merge?
There was a problem hiding this comment.
According to JsonProperty.Access javadoc: WRITE_ONLY means that the property may only be written (set) for deserialization, but will not be read (get) on serialization, that is, the value of the property is not included in serialization.
So, the existing "oraclebmcs" will be deserialized into PersistenceStorage and will be consolidated with the deserialized "oracle" to produce a new "oracle" for serialization. The serialized "persistentStorage" will not contain "oraclebmcs".
|
I can squash the commits as a part of the merge. |
oraclebmcs provider is renamed to oracle; oraclebmcs storage is renamed to oracle.