Add option to include causes in the JCasC export, and read causes from it on import. #139

allenbenz · 2021-01-30T00:51:22Z

This adds an option to write causes to the JCasC config.
This also allows causes to be read from the JCasC config and is not gated behind enabling the write to JCasC option.

My use case for this is to workaround an issue where how I deploy jenkins causes JCasC to rewrite the plugin configurations - which erases the local knowledge base.
There are other more valid use cases such as wanting to version control the causes list that this provides for

Made it an option as a few dozen causes makes the JCasC export very long.

Some concerns I have that I was unable to resolve:

I don't know what this will do to mongodb, I don't have a way to test that meaningfully.
The new Date(time) line in FailureCauseModification.java is parsing Date.toString() output. This should be safe as long as the locale of the jenkins master doesn't change.
The multilineBuildLogIndication JCasC "pattern" is wrong on export, it's the output of pattern.ToString() instead of the user provided one.

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java

olivergondza · 2021-03-30T13:29:22Z

Looking forward for this feature to be added in the plugin.

I have added some minor review comments.

olivergondza · 2021-09-13T11:47:33Z

@rsandell, @TWestling, folks, can you please take a look? Thanks!

rsandell · 2021-09-24T13:42:48Z

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java

+     * Get if causes from the current knowledge base should be included in the JCasC export is enabled.
+     * @return exportCausesToJCasCEnabled - on or off. null == off.
+     */
+    public boolean getExportCausesToJCasCEnabled() {


Java bean standard:

Suggested change

public boolean getExportCausesToJCasCEnabled() {

public boolean isExportCausesToJCasCEnabled() {

rsandell · 2021-09-24T13:44:00Z

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java

+     */
+    public Collection<FailureCause> getCauses() {
+        // The Causes export can be very large so we only include it if enabled.
+        if (!getExportCausesToJCasCEnabled()) {


Suggested change

if (!getExportCausesToJCasCEnabled()) {

if (!isExportCausesToJCasCEnabled()) {

Ah, I didn't realize JCasC's reflection would find methods starting with is.

It is the Java bean standard :)

rsandell · 2021-09-24T13:45:26Z

src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/FailureCause.java

     * @param comment       the comment of this FailureCause.
     * @param lastOccurred  the time at which this FailureCause last occurred.
     * @param categories    the categories of this FailureCause.
     * @param indications   the list of indications
     * @param modifications the modification history of this FailureCause.
     */
-    @DataBoundConstructor
    public FailureCause(String id, String name, String description, String comment,


Suggested change

public FailureCause(String id, String name, String description, String comment,

@Deprecated

public FailureCause(String id, String name, String description, String comment,

I don't know if this is really Deprecated, the fewer parameter DataBoundConstructor and the added DataBoundSetters allow JCasC to handle optional fields, but this constructor is a lot nicer to use in code.

Though if you still want it Deprecated that's fine too.

The deprecation in this scenario would only be a flag to anyone coding against it that it is not the one actually used in runtime. It can still be used in tests, just annotated with something if sportbugs or checkstyle complains.

rsandell

Sorry for the delay in reviewing. So much to do and so little time.

rsandell · 2021-09-24T13:50:43Z

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java

+        if (knowledgeBase != null) {
+            for (FailureCause cause:causes) {
+                try {
+                    knowledgeBase.saveCause(cause);


I am quite certain this will create duplicates for anything else than the file based knowledge base. Unless the id field is set. At least the javadoc should reflect that.

Writing a unit test for that should be relatively easy. There are a couple of integration tests using an embedded mongodb that you can take inspiration from.

I've added a note about duplicates to the javadoc, there isn't much I can do about missing ids short of skipping entries that don't have one, thoughts on the preferred behavior?

…xternal resource.

allenbenz · 2021-10-01T20:20:36Z

So I'm having a bit of a hygiene problem with the port embedded mongodb runs on.
It would be preferable to allow the OS to provide one to avoid colliding with some other process.

But I'm not able to think of a good way to inject that port number into the config, it's a resource so I can't change it at runtime, and there aren't any hooks in JenkinsConfiguredWithCodeRule or ConfigurationAsCode to apply a transform after it gets loaded but before it's parsed.

Only improvement I could think of would be to swap 27017 with something that's not the mongodb default port to lower the odds of collision but that's not a real solution.

Thoughts?

olivergondza · 2021-10-02T14:54:14Z

@allenbenz, use ConfigurationAsCode.get().configure() or ConfigurationAsCode.get().configureWith() without the annotation. You can pre-process the input the way you like.

yuezhuangshi · 2023-04-14T06:31:28Z

Looking forward for this feature to be added in the plugin.

jhaxon · 2023-06-01T00:26:10Z

We're unable to deploy this plugin across our fleet without this functionality.

rsandell · 2023-07-14T10:29:56Z

@jhaxon As it seems like the original author has abandoned the change anyone is welcome to continue to work on it to get it over the finish line :)

carlosgv87 · 2024-10-15T13:00:05Z

@allenbenz , @rsandell is there any plan to include this feature on further versions of this plugin?

Thank you in advance 😃

allenbenz · 2024-10-15T18:46:45Z

@carlosgv87 unfortunately I have long since stopped working at the place where I was using Jenkins. If someone else wants to pick this up and finish it they should feel welcome to do so. My changes are MIT licensed so ownership isn't a concern for the next person.

allenbenz changed the title ~~Add feature to optionally include causes in the JCasC export~~ Add option to include causes in the JCasC export Feb 2, 2021

allenbenz changed the title ~~Add option to include causes in the JCasC export~~ Add option to include causes in the JCasC export, and read causes from it on import. Feb 3, 2021

allenbenz force-pushed the add-causes-to-jcasc branch from 66870b5 to e737d80 Compare February 18, 2021 01:34

allenbenz force-pushed the add-causes-to-jcasc branch from e737d80 to c5d4228 Compare March 18, 2021 06:53

olivergondza reviewed Mar 30, 2021

View reviewed changes

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java Outdated Show resolved Hide resolved

olivergondza reviewed Mar 30, 2021

View reviewed changes

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java Outdated Show resolved Hide resolved

olivergondza reviewed Mar 30, 2021

View reviewed changes

src/main/java/com/sonyericsson/jenkins/plugins/bfa/PluginImpl.java Show resolved Hide resolved

allenbenz force-pushed the add-causes-to-jcasc branch from d6c622b to c260c2a Compare April 19, 2021 18:34

rsandell added the enhancement label Sep 24, 2021

rsandell reviewed Sep 24, 2021

View reviewed changes

rsandell requested changes Sep 24, 2021

View reviewed changes

allenbenz added 7 commits September 30, 2021 13:59

Add feature to optionally include causes in the JCasC export

3c10207

satisfy javadoc lints.

856aee4

Use boolean instead of Boolean for exportCausesToJCasCEnabled

6be3f41

Better logging for get/setCauses exception

4a864a1

get -> is, works with JCasC and is to the java bean standard

f544c1c

Be clear that setCauses duplicates causes if ids are absent.

2757966

Add JCasC export causes tests for mongodb

b35c395

allenbenz force-pushed the add-causes-to-jcasc branch from c260c2a to b35c395 Compare October 1, 2021 00:12

Run tests in ConfigurationAsCodeMongotTest with an embedded mongodb e…

39d287f

…xternal resource.

allenbenz force-pushed the add-causes-to-jcasc branch from fd9db07 to 39d287f Compare October 1, 2021 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to include causes in the JCasC export, and read causes from it on import. #139

Add option to include causes in the JCasC export, and read causes from it on import. #139

allenbenz commented Jan 30, 2021 •

edited

Loading

olivergondza commented Mar 30, 2021

olivergondza commented Sep 13, 2021

rsandell Sep 24, 2021

rsandell Sep 24, 2021

allenbenz Sep 30, 2021

rsandell Feb 4, 2022

rsandell Sep 24, 2021

allenbenz Sep 30, 2021

rsandell Feb 4, 2022

rsandell left a comment

rsandell Sep 24, 2021

rsandell Sep 24, 2021

allenbenz Oct 1, 2021

allenbenz commented Oct 1, 2021

olivergondza commented Oct 2, 2021

yuezhuangshi commented Apr 14, 2023

jhaxon commented Jun 1, 2023

rsandell commented Jul 14, 2023

carlosgv87 commented Oct 15, 2024

allenbenz commented Oct 15, 2024

	public boolean getExportCausesToJCasCEnabled() {
	public boolean isExportCausesToJCasCEnabled() {

	if (!getExportCausesToJCasCEnabled()) {
	if (!isExportCausesToJCasCEnabled()) {

	public FailureCause(String id, String name, String description, String comment,
	@Deprecated
	public FailureCause(String id, String name, String description, String comment,

Add option to include causes in the JCasC export, and read causes from it on import. #139

Are you sure you want to change the base?

Add option to include causes in the JCasC export, and read causes from it on import. #139

Conversation

allenbenz commented Jan 30, 2021 • edited Loading

olivergondza commented Mar 30, 2021

olivergondza commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenbenz commented Oct 1, 2021

olivergondza commented Oct 2, 2021

yuezhuangshi commented Apr 14, 2023

jhaxon commented Jun 1, 2023

rsandell commented Jul 14, 2023

carlosgv87 commented Oct 15, 2024

allenbenz commented Oct 15, 2024

allenbenz commented Jan 30, 2021 •

edited

Loading