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

Added a new DatabaseChoiceListProvider with ja/fr/en translations #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tduchateau
Copy link

  • The new choice list provider is fully configurable: JDBC URL and driver, db name, db table, db column, db username, db password. It allows per-job database configuration only.
  • Moreover, a fallback mechanism has been added and allows to read a local file containing choices (one per line) if the connection to the database can't be established (for any reason)
  • All Messages*.properties have been updated accordingly
  • WARNING: the driver class must be manually added to the classpath (either by putting the right JAR in the WEB-INF\lib folder of Jenkins, or by installing one of the Database plugins, such as https://wiki.jenkins-ci.org/display/JENKINS/MySQL+Database+Plugin)

* The new choice list provider is fully configurable: JDBC URL and driver, db name, db table, db column, db username, db password
* Moreover, a fallback mechanism has been added and allows to read a local file containing choices (one per line) if the connection to the database can't be established (for any reason)
* All Messages*.properties have been updated accordingly
* WARNING: the driver class must be manually added to the classpath (either by putting the JAR in the WEB-INF\ib folder of Jenkins, or by installing one the Database Plugin such as https://wiki.jenkins-ci.org/display/JENKINS/MySQL+Database+Plugin
@tduchateau
Copy link
Author

Additional notes:

  • There is no tests. Shame on me :-(
  • mvn test launched locally in the original code base, before any modification, was already failing with Tests run: 70, Failures: 0, Errors: 35, Skipped: 0. I didn't take the time to try to fix them.
  • I had to comment some @Override annotations in order to fix compiling issues. Compiler used in Eclipse: 1.6.
  • This one could be a showstopper: I had to upgrade the org.jenkins-ci.plugins:plugin from 1.466 to 1.580.1.But because I'm not sure this is the minimal version required, I didn't push the pom.xml file
  • Finally, I had to explicitely use the latest release of the maven-hpi-plugin by adding the following to the pom.xml file:
<build> 
   <plugins> 
      <plugin> 
         <groupId>org.jenkins-ci.tools</groupId> 
         <artifactId>maven-hpi-plugin</artifactId> 
         <version>1.95</version> 
      </plugin> 
   </plugins> 
</build>

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

}
}
catch (SQLException se) {
LOGGER.log(Level.SEVERE,"Unable to access the database: " + dbName + "." + se);
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Logger.log(Level, String, Throwable).
It displays also stacktraces.

@ikedam
Copy link
Member

ikedam commented Jan 13, 2015

mvn eclipse:eclipse generates build configurations that force to compile with Java 1.5.
That cause compilation errors as annotations (like @Override) are introduced in Java1.6.
Uncheck a checkbox like "project specific configuraion" in Project -> Properties -> Java Compiler and you can compile that without errors.

@ikedam
Copy link
Member

ikedam commented Jan 13, 2015

uncheckhere

private String jdbcDriver;
private String fallbackFilePath;

public String getDbUsername() {
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent indentation.
Please don't use tab letters for indentations. Use only blank letters for that.

Copy link
Author

Choose a reason for hiding this comment

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

Please send me you code style formatter. Much more simpler for me.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's a problem with a formatter.
It's generally a bad idea to use both blank characters and tab characters for indentations.
You'd better to configure your Eclipse to display whitespace characters.

If you need a formatter, I recommend you to create a one with following pages.
https://wiki.jenkins-ci.org/display/JENKINS/Governance+Document#GovernanceDocument-CoreCodingConvention
http://www.oracle.com/technetwork/java/codeconvtoc-136057.html

@ikedam
Copy link
Member

ikedam commented Jan 15, 2015

WARNING: the driver class must be manually added to the classpath (either by putting the right JAR in the WEB-INF\lib folder of Jenkins, or by installing one of the Database plugins, such as https://wiki.jenkins-ci.org/display/JENKINS/MySQL+Database+Plugin)

This can be resolved by integrated with Database plugin.
Though there are too few examples, you can integrate that as a following way (just outline):

  1. add an optional dependency to database-plugin 1.1.
    • As 1.1 is the greatest version that works with Jenkins 1.466 targeted by extensible-choice-parameter plugin.
    • Of course you can upgrade the target Jenkins version and depends to the latest database plugin, but I don't think we need features of database plugin introduced in 1.2.
  2. Replace database connaction parameter fields of DatabaseChoiceList provider with Database.
  3. in config.jelly:
    <f:dropdownDescriptorSelector field="database" title="${%Database}" />

@ikedam
Copy link
Member

ikedam commented Jan 31, 2015

#9 demonstrates implementation with database plugin.

}

@DataBoundConstructor
public DatabaseChoiceListProvider(String userdb, String passworddb, String urldb, String colomndb, String namedb, String tabledb, String driverdb, String fallbackfile)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't work as Jenkins binds parameters in jelly and parameters in the constructer with their definition name.
(they should be `String dbUsername, String dbPassword, ...)

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.

3 participants