-
Notifications
You must be signed in to change notification settings - Fork 148
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
Adding support for DynamoDB backend #85
base: master
Are you sure you want to change the base?
Conversation
d58bfff
to
c0284c9
Compare
@@ -2,7 +2,7 @@ | |||
<modelVersion>4.0.0</modelVersion> | |||
<groupId>com.sonyericsson.jenkins.plugins.bfa</groupId> | |||
<artifactId>build-failure-analyzer</artifactId> | |||
<version>1.19.3-SNAPSHOT</version> | |||
<version>1.20.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't change this. I'll update the version when releasing the plugin
*/ | ||
@DataBoundConstructor | ||
public DynamoDBKnowledgeBase(String region, String credentialsPath, String credentialsProfile) { | ||
if (region == null || region.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit StringUtils.isEmpty()
could be used instead.
convertFromAbstract(oldKnowledgeBase); | ||
convertRemoved((DynamoDBKnowledgeBase)oldKnowledgeBase); | ||
} else { | ||
for (FailureCause cause : oldKnowledgeBase.getCauseNames()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getCauseNames()
doesn't give you all the data, so the conversion will loose information.
@Override | ||
public int hashCode() { | ||
//Making checkstyle happy. | ||
return getClass().getName().hashCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't conform to the "equals and hashCode" style, i.e. if two objects are equal they should also have the same hashcode and if two Object's hashcodes are equal the Objects should also be equal.
So add region
, credentialsPath
and credentialsProfile
as well. Most moders IDEs should be able to do that for you.
* @param credentialProfile the credential profile to use. | ||
* @return {@link FormValidation#ok() } if can be done, | ||
* {@link FormValidation#error(java.lang.String) } otherwise. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequirePOST
see this week's security advisory many plugins have this issue ;)
@Override | ||
public int hashCode() { | ||
//Making checkstyle happy. | ||
return getClass().getName().hashCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same equals and hashCode problems here, but in this case just looking at the id
might be enough.
Indication indication = Indication.class.cast(obj); | ||
indications.add(indication); | ||
} catch (Exception e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be properly logged via for example logger
.
@Override | ||
public int hashCode() { | ||
//Making checkstyle happy. | ||
return getClass().getName().hashCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dito.
<!-- | ||
~ The MIT License | ||
~ | ||
~ Copyright 2012 Sony Mobile Communications AB. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be changed to yours
* @param value the pattern to check. | ||
* @return {@link hudson.util.FormValidation#ok()} if everything is well. | ||
*/ | ||
public FormValidation doCheckCredentialsPath(@QueryParameter("value") final String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first off this should use proper credentials via the credentials store instead ( the old mongodb code was done before we had any real knowledge of how Jenkins credentials worked :) )
Second this is a potential security risk as it can be used by anyone with overall/read permission to query the existence of a file on the Jenkins master file system. So needs @RequirePOST
and a Jenkins.checkPermission(Jenkins.ADMINISTER)
. The jelly also needs to be updated so the check is done via POST instead of a GET.
Thanks for the contribution, sorry that I'm taking so much time doing reviews. |
@citizenken Any update on this ? We were also looking towards DynamoDB support for this plugin |
1 similar comment
@citizenken Any update on this ? We were also looking towards DynamoDB support for this plugin |
What
This PR implements a DynamoDB backend for FailureCause storage. It makes the following changes:
From a user's perspective, the UI/modifying failures/how failures are displayed is unchanged.
From a DB perspective, I have ensured that the data structure is the same as the Mogo structure:
Why
When evaluating the plugin for use, my team really liked the concept of a dedicated database for tracking failure info. However, without more support outside of our team, we could not justify the operationalizing/maintenance cost of a MongoDB cluster. We're not DBA's, so it falls out of our wheelhouse. Additionally, the plugin uses the mongo-java-driver 2.x, which only communicates with MongoDB v2.x, which has now been EoL'd.
For a no/low-maintenance NoSQL store, we want to use DynamoDB. It is easy to operationalize, and we don't have to dedicate much time/thought into management of the database. As our company is moving more to AWS services, it seems like a natural fit, and could be a good fit for other teams as well.
Testing
I've written a testsuite for the new backend, as well as tests for any functionality I've added. The backend testsuite has a fairly high level of mocking, mostly because of how much DynamoDB's Java library handles behind the scenes.
ToDo
This is a large PR because it adds the bulk of the functionality (CRUD operations). That said, there is still more to implement to reach Mongo parity:
UI screenshot