Partialy restore binary compatibility with 2.13 and older releases #29
Conversation
olivergondza
changed the title
|
@imod, this will make update to 2.14+ much less painful. |
olivergondza
changed the title
| * @deprecated Use <tt>GlobalConfigFiles.get().getConfigs()</tt> instead. | ||
| */ | ||
| @Deprecated | ||
| public Collection<Config> getAllConfigs() { |
There was a problem hiding this comment.
although this makes the API some how compatible again, it is semantically wrong. GlobalConfigFiles.get().getConfigs(); just returns the global configuration files and none in any folders. So I don't think this is a good idea.
The same applies to all the other methods...
There was a problem hiding this comment.
If you think this is a good solution for the openstack extention of the config-file-provider, then I think you should just access GlobalConfigFiles.get() directly
There was a problem hiding this comment.
maybe I judged to fast, let me think about it again - sorry...
There was a problem hiding this comment.
The correct way to access configuration files is via org.jenkinsci.plugins.configfiles.ConfigFiles and not via GlobalConfigFiles - GlobalConfigFiles should never be accessed directly.
Readding these methods to this class for sure cause wrong usage.
So I'm really puzzled about this change...
There was a problem hiding this comment.
I like this approach. It would also restore compatibility for Job DSL.
But getAllConfigs() should only return the configs managed by this provider. See https://github.com/jenkinsci/config-file-provider-plugin/blob/config-file-provider-2.13/src/main/java/org/jenkinsci/lib/configprovider/ConfigProvider.java#L77-L82. This change would return all global configs.
|
To my understanding, in past there ware only global files and now you have added support for folder specific files as well. The migration just tool all the files and moved it to "global space". These methods I put back used to return global files and that is what I want to preserve as plugins that was not adjusted to this change expect exactly that. What do you think? |
|
Looking at it from that perspective does actually make it quite reasonable. But then I guess the message next to |
|
@imod, @daspilker, can we agree on getting these methods back mimicking the old behavior and cutting the release ASAP? I really seems that I return all configs, not only the ones related to given provider. Will fix. |
|
I have updated the code to list only methods bound to this provider. The rest is manipulating configs by id that is documented to be unique accross providers so it should be fine. |
|
👍 I will merge this |
|
@olivergondza thanks!! |
|
Can you release it today? I have started working on a reflexion-based workaround for openstack-cloud to support 2.15 - 2.15.3 but it might not be necessary if we get 2.15.4 soon enough. Thanks |
|
@olivergondza yes, I will release it today |
|
@olivergondza version |
The tests are passing for me in openstack-cloud-plugin without any modification there. I will give it a bit more manual testing with old and new c-f-p and report here. Anyway, I really believe the compatibility must be preserved here.