Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Optimize JobLocalConfiguration #321

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

jglick
Copy link
Member

@jglick jglick commented Aug 2, 2024

Amends #113, which was rewriting every job config.xml during every save, even in the normal case that changeReasonComment was left blank. Noticed because the config.xml was being left in a half-written state on occasion; using AtomicFileWriter would help, though better still would be to use a different system entirely for recording change reason comments (like just keeping this information in memory).

@jglick jglick requested a review from a team as a code owner August 2, 2024 21:11
@jglick
Copy link
Member Author

jglick commented Aug 9, 2024

@jenkinsci/jobconfighistory-plugin-developers is empty. @basil @NotMyFault you seem to have write access?

@jglick
Copy link
Member Author

jglick commented Aug 23, 2024

The fact that this is effective in suppressing the unwanted rewrite

diff --git src/main/java/hudson/plugins/jobConfigHistory/FileHistoryDao.java src/main/java/hudson/plugins/jobConfigHistory/FileHistoryDao.java
index 00109eb..621f9bb 100644
--- src/main/java/hudson/plugins/jobConfigHistory/FileHistoryDao.java
+++ src/main/java/hudson/plugins/jobConfigHistory/FileHistoryDao.java
@@ -400,7 +400,7 @@ public class FileHistoryDao extends JobConfigHistoryStrategy
             } else return Optional.empty();
         } else {
             //no jobLocalConfiguration node found. Should be there even if the message field was empty!
-            LOG.log(FINEST, "tag \"{0}\" not found in {1}, no comment could be found.",
+            LOG.log(Level.INFO, "tag \"{0}\" not found in {1}, no comment could be found.",
                     new Object[]{JobConfigHistoryConsts.JOB_LOCAL_CONFIGURATION_XML_TAG, configFile.getFile()});
             return Optional.empty();
         }
diff --git src/main/java/hudson/plugins/jobConfigHistory/JobLocalConfiguration.java src/main/java/hudson/plugins/jobConfigHistory/JobLocalConfiguration.java
index 6305740..827ce3b 100644
--- src/main/java/hudson/plugins/jobConfigHistory/JobLocalConfiguration.java
+++ src/main/java/hudson/plugins/jobConfigHistory/JobLocalConfiguration.java
@@ -32,7 +32,7 @@ public class JobLocalConfiguration extends JobProperty<Job<?, ?>> {
         return "";
     }
 
-    @Extension
+    //@Extension
     public static class DescriptorImpl extends JobPropertyDescriptor {
 
         public DescriptorImpl() {

(confirmed by both logging and lack of unexpected XML prologue in job config.xml) suggests that a workaround for anyone running a current release is to install https://plugins.jenkins.io/extension-filter/ and block hudson.plugins.jobConfigHistory.JobLocalConfiguration$DescriptorImpl, though I have not tried that directly.

Copy link
Contributor

@StefanSpieker StefanSpieker left a comment

Choose a reason for hiding this comment

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

It looks reasonable, without being an expert (yet).

@StefanSpieker StefanSpieker merged commit 07634fa into jenkinsci:master Aug 24, 2024
16 checks passed
@jglick jglick deleted the JobLocalConfiguration branch August 24, 2024 14:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants