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.

Sign up for GitHub

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

Jump to bottom

Partialy restore binary compatibility with 2.13 and older releases #29

Merged
merged 2 commits into from Jan 13, 2017
Merged

Partialy restore binary compatibility with 2.13 and older releases #29

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ceb39da
Partialy restore binary compatibility with 2.13 and older releases
olivergondza Jan 12, 2017
bba752a
List only relevant configs. Improve javadoc.
olivergondza Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions src/main/java/org/jenkinsci/lib/configprovider/ConfigProvider.java
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ of this software and associated documentation files (the "Software"), to deal
import org.jenkinsci.lib.configprovider.model.Config;
import org.jenkinsci.lib.configprovider.model.ContentType;
import org.jenkinsci.plugins.configfiles.ConfigFileStore;
import org.jenkinsci.plugins.configfiles.GlobalConfigFiles;

import java.util.Collection;

Expand Down Expand Up @@ -128,4 +129,46 @@ public boolean supportsFolder() {
return true;
}


/**
* @deprecated Use <tt>GlobalConfigFiles.get().getConfigs()</tt> instead.
*/
@Deprecated
public Collection<Config> getAllConfigs() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I judged to fast, let me think about it again - sorry...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return GlobalConfigFiles.get().getConfigs();
}

/**
* @deprecated Use <tt>GlobalConfigFiles.get().getById(String)</tt> instead.
*/
@Deprecated
public Config getConfigById(String configId) {
return GlobalConfigFiles.get().getById(configId);
}

/**
* @deprecated Use <tt>GlobalConfigFiles.get().getById(String)</tt> instead.
*/
@Deprecated
public boolean configExists(String configId) {
return GlobalConfigFiles.get().getById(configId) != null;
}

/**
* @deprecated Use <tt>GlobalConfigFiles.get().remove(String)</tt> instead.
*/
@Deprecated
public void remove(String configId) {
GlobalConfigFiles.get().remove(configId);
this.save();
}

/**
* @deprecated Use <tt>GlobalConfigFiles.get().save(Config)</tt> instead.
*/
@Deprecated
public void save(Config config) {
GlobalConfigFiles.get().save(config);
this.save();
}
}