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

Allows for multiple deserialization class renames files #144

Closed
wants to merge 1 commit into from

Conversation

nakomis
Copy link
Contributor

@nakomis nakomis commented May 18, 2016

No description provided.

@aledsage
Copy link
Contributor

Worth adding test case(s).
(ping me offline if you want to talk about how best to write good unit-test coverage for this).

Also note the related discussion the mailing list. An alternative suggestion is that we pick up all files with the given name on the classpath (rather than all those in a given directory). I personally prefer the approach of all files in the directory. That would allow us to more easily incrementally add things (e.g. have separate files for upgrading between versions).


I wonder about a nicer package name than org.apache.brooklyn.core.mgmt.persist.deserializingClassRenames. I imagine many people will just put this in their ./conf/ directory, so we don't want them to have to create a really deep nested directory.


Another thing we could add (in the future?) is if there are conflicting changes - e.g. A is renamed to B in the first file, and B is renamed to C in the second file. Currently, the result would depend on the other the files were processed: i.e. it could be "B" or "C". It would be good to be more predictable.

I'm fine with that being deferred for now.

@ahgittin
Copy link
Contributor

ahgittin commented Sep 9, 2016

@nakomis ping

@tbouron
Copy link
Member

tbouron commented Feb 7, 2017

Hey @nakomis, overloading the class deserialisation has been addressed in #492 but for karaf edition only. Do you also need it for the classic edition? In this case, could you rebase on the latest master + add tests?

@nakomis
Copy link
Contributor Author

nakomis commented Feb 7, 2017

Not needed for classic, so #492 should suffice, closing

@nakomis nakomis closed this Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants