[JENKINS-63587] Allow disabling Ant default excludes in the zip step #180

ykhandelwal913 · 2023-01-09T04:09:21Z

Bydefault zipcreation excludes the default files like .gitignore and others mentioned in https://ant.apache.org/manual/dirtasks.html#defaultexcludes.

This PR is to solve https://issues.jenkins.io/browse/JENKINS-63587

Example:
zip zipFile: 'template.zip', archive: true, defaultExcludes:false, glob: 'template-*/**/*'
default value of defaultExcludes is true.

rsandell

Missing documentation

rsandell · 2023-01-09T15:23:03Z

See https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep and https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/tar/TarStep

Also if this only applies to zip files then the parameter should be added to that step, not the abstract step that also is used by TarStep.

ykhandelwal913 · 2023-01-10T08:45:08Z

See https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep and https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/tar/TarStep

Also if this only applies to zip files then the parameter should be added to that step, not the abstract step that also is used by TarStep.

It is only applies to zip files.

ykhandelwal913 · 2023-01-10T08:45:34Z

Missing documentation

Done. Added in code itself. is that fine?

rsandell · 2023-01-10T16:31:15Z

You also need to add a help-defaultExcludes.html to the resource directory https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep with user documentation explaining what it is about, not every user can read java code :)

ykhandelwal913 · 2023-01-11T02:05:14Z

You also need to add a help-defaultExcludes.html to the resource directory https://github.com/jenkinsci/pipeline-utility-steps-plugin/tree/master/src/main/resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep with user documentation explaining what it is about, not every user can read java code :)

Done

ykhandelwal913 · 2023-01-12T01:19:26Z

@rsandell not sure why build is failing. Can you help?

rsandell · 2023-01-12T15:10:14Z

Looks like an update of the build library causing javadoc generation to run and fail, not your fault.

ykhandelwal913 · 2023-01-12T16:11:17Z

@rsandell If PR looking good, can that be merged?

.../resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep/help-defaultExclude.html

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCompressStep.java

basil · 2023-01-13T15:18:01Z

Looks like the build failure is caused by a Javadoc error:

16:49:16  [ERROR] /home/jenkins/agent/workspace/line-utility-steps-plugin_PR-180/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCompressStep.java:132: error: invalid use of @return
16:49:16  [ERROR]      * @return the defaultExcludes boolean
16:49:16  [ERROR]        ^

basil · 2023-01-13T15:19:28Z

PS You can test Javadoc generation locally by running mvn clean verify -DskipTests -Pjenkins-release.

…tility-steps-plugin

ykhandelwal913 · 2023-01-13T15:30:38Z

Looks like the build failure is caused by a Javadoc error:

16:49:16  [ERROR] /home/jenkins/agent/workspace/line-utility-steps-plugin_PR-180/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCompressStep.java:132: error: invalid use of @return
16:49:16  [ERROR]      * @return the defaultExcludes boolean
16:49:16  [ERROR]        ^

Thanks for the pointer. Fixed it.

basil · 2023-01-13T15:43:48Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStepExecution.java

@@ -75,10 +75,13 @@ static class ZipItFileCallable extends AbstractFileCallable<Integer> {
        final String exclude;
        final boolean overwrite;

-        public ZipItFileCallable(String glob, String exclude, boolean overwrite) {
+        private boolean defaultExcludes = true;


Making this field final would make it both consistent with the other fields in this class and easier to reason about (mutable state is generally harder to reason about than immutable state).

Done. can you review?

basil · 2023-01-13T16:55:10Z

.../resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep/help-defaultExclude.html

+<p>
+    The zip file should include defaultExcludes provided by ant java pattern.
+    By Default defaultExcludes param value will be true.
+</p>


This documentation is grammatically confusing (the first sentence probably meant to say "Whether the zip file should […]") and also has incorrect capitalization. Suggest clarifying as follows:

<p>By default, the following exclusions are applied:</p> <pre>**/*~ **/#*# **/%*% **/.#* **/._* **/.bzr **/.bzr/** **/.bzrignore **/CVS **/CVS/** **/.cvsignore **/.DS_Store **/.git **/.git/** **/.gitattributes **/.gitignore **/.gitmodules **/.hg **/.hg/** **/.hgignore **/.hgsub **/.hgsubstate **/.hgtags **/SCCS **/SCCS/** **/.svn **/.svn/** **/vssver.scc </pre> <p>Set this option to <code>false</code> if you do not want these default exclusions to be applied.</p>

basil · 2023-01-13T16:55:27Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep.java

+     * <a href="https://ant.apache.org/manual/dirtasks.html#defaultexcludes" target="_blank">Ant style pattern</a>
+     * of files to disable default excludes from the archive.


Incorrect: this is a boolean, not a pattern.

basil · 2023-01-13T16:55:34Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep.java

+     * <a href="https://ant.apache.org/manual/dirtasks.html#defaultexcludes" target="_blank">Ant style pattern</a>
+     * of files to disable default excludes from the archive.


Incorrect: this is a boolean, not a pattern.

basil · 2023-01-13T17:05:31Z

It is only applies to zip files.

From my reading of the code, it appears the tar step uses the same Ant task and therefore the same default exclusions. If true, then it seems inconsistent to add this parameter to the zip step but not the tar step.

ykhandelwal913 · 2023-01-13T18:41:41Z

From my reading of the code, it appears the tar step uses the same Ant task and therefore the same default exclusions. If true, then it seems inconsistent to add this parameter to the zip step but not the tar step.

As this ticket was for zip step i added code only for that but i agree with your point and to have the same consistency in zip and tar, added the same in tarStep as well.

ykhandelwal913 · 2023-01-14T16:19:13Z

@basil can this be reviewed now. This feature is much needed for us to progress in our project. Hence the urgency.

basil

This is looking good overall, but I think now that the scope of this PR extends to both the zip and the tar step, HTML documentation for the tar step needs to be added to match that of the zip step, along with a corresponding test for the tar step. Otherwise the change is inconsistent, applying some logic to one step but not the other.

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep.java

basil

Looks good to me. Thanks for your contribution and for addressing feedback so quickly! In order to set expectations properly, I am not a maintainer of this plugin (though I am a Jenkins core maintainer), so I will be unable to merge your change. But @rsandell is a maintainer of this plugin, and I believe that the work we have done together has brought this PR closer to a state that can be approved by him (if not to the finish line!). Thanks again!

ykhandelwal913 · 2023-01-15T02:14:36Z

Looks good to me. Thanks for your contribution and for addressing feedback so quickly! In order to set expectations properly, I am not a maintainer of this plugin (though I am a Jenkins core maintainer), so I will be unable to merge your change. But @rsandell is a maintainer of this plugin, and I believe that the work we have done together has brought this PR closer to a state that can be approved by him (if not to the finish line!). Thanks again!

Thanks for the feedback and support provided.

rsandell · 2023-01-16T12:11:26Z

Looks good, from what I can see you only need to fix the javadoc errors in the new code to make the build pass.

ykhandelwal913 · 2023-01-16T16:05:36Z

Looks good, from what I can see you only need to fix the javadoc errors in the new code to make the build pass.

Done

ykhandelwal913 · 2023-01-16T17:13:03Z

@rsandell can this be merged?

ykhandelwal913 · 2023-01-17T03:51:22Z

@rsandell It didnot create a new release. Do i need to do anything from my side?

Allowing default excludes

6dc17ce

rsandell approved these changes Jan 9, 2023

View reviewed changes

rsandell added the enhancement label Jan 9, 2023

rsandell requested changes Jan 9, 2023

View reviewed changes

updating feedback comments and documentation

242a047

updating feedback comments and documentation

6cbe2c9

Adding unit tests

adfb110

rsandell requested changes Jan 13, 2023

View reviewed changes

.../resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep/help-defaultExclude.html Outdated Show resolved Hide resolved

Merge branch 'master' into master

65b98a9

rsandell requested changes Jan 13, 2023

View reviewed changes

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCompressStep.java Show resolved Hide resolved

yogeshkhandelwal added 2 commits January 13, 2023 20:58

Moving code to zipstep class

343ddb8

Merge branch 'master' of https://github.com/ykhandelwal913/pipeline-u…

888d4a2

…tility-steps-plugin

Moving code to zipstep class

1d960c5

basil reviewed Jan 13, 2023

View reviewed changes

Making defaultExcludes final

bb5afc4

basil reviewed Jan 13, 2023

View reviewed changes

adding default Excludes for tar

e16dd4a

basil reviewed Jan 14, 2023

View reviewed changes

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStep.java Outdated Show resolved Hide resolved

yogeshkhandelwal added 2 commits January 15, 2023 07:21

Adding tar docs and unit tests

a869835

Removing unused import

57c8324

basil approved these changes Jan 15, 2023

View reviewed changes

rsandell approved these changes Jan 16, 2023

View reviewed changes

fixing javadoc

3f0a7d6

rsandell changed the title ~~JENKINS-63587: Allow disabling Ant default excludes in the zip step~~ [JENKINS-63587] Allow disabling Ant default excludes in the zip step Jan 16, 2023

rsandell merged commit f7fd56c into jenkinsci:master Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-63587] Allow disabling Ant default excludes in the zip step #180

[JENKINS-63587] Allow disabling Ant default excludes in the zip step #180

ykhandelwal913 commented Jan 9, 2023 •

edited

Loading

rsandell left a comment

rsandell commented Jan 9, 2023

ykhandelwal913 commented Jan 10, 2023

ykhandelwal913 commented Jan 10, 2023

rsandell commented Jan 10, 2023

ykhandelwal913 commented Jan 11, 2023

ykhandelwal913 commented Jan 12, 2023

rsandell commented Jan 12, 2023

ykhandelwal913 commented Jan 12, 2023 •

edited

Loading

basil commented Jan 13, 2023

basil commented Jan 13, 2023

ykhandelwal913 commented Jan 13, 2023

basil Jan 13, 2023

ykhandelwal913 Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil Jan 13, 2023

basil commented Jan 13, 2023 •

edited

Loading

ykhandelwal913 commented Jan 13, 2023

ykhandelwal913 commented Jan 14, 2023

basil left a comment

basil left a comment

ykhandelwal913 commented Jan 15, 2023

rsandell commented Jan 16, 2023

ykhandelwal913 commented Jan 16, 2023

ykhandelwal913 commented Jan 16, 2023

ykhandelwal913 commented Jan 17, 2023 •

edited

Loading

		* <a href="https://ant.apache.org/manual/dirtasks.html#defaultexcludes" target="_blank">Ant style pattern</a>
		* of files to disable default excludes from the archive.

[JENKINS-63587] Allow disabling Ant default excludes in the zip step #180

[JENKINS-63587] Allow disabling Ant default excludes in the zip step #180

Conversation

ykhandelwal913 commented Jan 9, 2023 • edited Loading

rsandell left a comment

Choose a reason for hiding this comment

rsandell commented Jan 9, 2023

ykhandelwal913 commented Jan 10, 2023

ykhandelwal913 commented Jan 10, 2023

rsandell commented Jan 10, 2023

ykhandelwal913 commented Jan 11, 2023

ykhandelwal913 commented Jan 12, 2023

rsandell commented Jan 12, 2023

ykhandelwal913 commented Jan 12, 2023 • edited Loading

basil commented Jan 13, 2023

basil commented Jan 13, 2023

ykhandelwal913 commented Jan 13, 2023

basil Jan 13, 2023

Choose a reason for hiding this comment

ykhandelwal913 Jan 13, 2023

Choose a reason for hiding this comment

basil Jan 13, 2023

Choose a reason for hiding this comment

basil Jan 13, 2023

Choose a reason for hiding this comment

basil Jan 13, 2023

Choose a reason for hiding this comment

basil commented Jan 13, 2023 • edited Loading

ykhandelwal913 commented Jan 13, 2023

ykhandelwal913 commented Jan 14, 2023

basil left a comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

ykhandelwal913 commented Jan 15, 2023

rsandell commented Jan 16, 2023

ykhandelwal913 commented Jan 16, 2023

ykhandelwal913 commented Jan 16, 2023

ykhandelwal913 commented Jan 17, 2023 • edited Loading

ykhandelwal913 commented Jan 9, 2023 •

edited

Loading

ykhandelwal913 commented Jan 12, 2023 •

edited

Loading

basil commented Jan 13, 2023 •

edited

Loading

ykhandelwal913 commented Jan 17, 2023 •

edited

Loading