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

Allow copy and delete of ant default exclude files #105

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

RandomInEqualities
Copy link
Contributor

@RandomInEqualities RandomInEqualities commented Jul 6, 2024

Hi,

I recently had to setup at build machine and noticed that it is not possible to copy or delete .gitignore files.
We are using this plugin for file operations, so it is a bit annoying and I thought I would try a pull request.

Currently, it is not possible to copy or delete files with the following names when using FileCopyOperation and FileDeleteOperation:

CVS
.cvsignore
SCCS
vssver.scc
.svn
.DS_Store
.git
.gitattributes
.gitignore
.gitmodules
.hg
.hgignore
.hgsub
.hgsubstate
.hgtags
.bzr
.bzrignore

Since the glob Ant pattern that FilePath::list and FilePath::copyRecursiveTo uses cannot pick up the files. This change changes to not use the default exclude list, so e.g. it is possible to copy and delete .gitignore files.

The Ant default exclude files are listed here:
https://ant.apache.org/manual/dirtasks.html#defaultexcludes

Testing done

I have added two automated tests that fail before this change, and pass after the change.

Submitter checklist

  • [ x] Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • [ x] Ensure that the pull request title represents the desired changelog entry
  • [x ] Please describe what you did
  • [x ] Link to relevant issues in GitHub or Jira
  • [x ] Link to relevant pull requests, esp. upstream and downstream changes
  • [x ] Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jonesbusy
Copy link
Contributor

Hi,

Thanks for your PR.

From what I saw it's a breaking change for users.

Could we add an optional option to those steps to not use default exclude if needed ?

Thanks.

@RandomInEqualities
Copy link
Contributor Author

Hi,

Year sure. I am not too familiar with Jenkins plugins. Would adding overloaded functions to FileOperationsJobDslContext work? Something like this:

image

? In the same way would you prefer to have the input to the FileCreateOperation, FileCopyOperation and FileTransformOperation as constructor parameters or setter functions?

@RandomInEqualities
Copy link
Contributor Author

Ideally I think a plugin wide boolean check would make sense, if that is possible? So it can be turned on an off for the whole plugin and not per function.

@jonesbusy
Copy link
Contributor

Hi,

I think the best is to use https://javadoc.jenkins.io/component/stapler/org/kohsuke/stapler/DataBoundSetter.html

Generally mandatory argument are passed on the constructor via https://javadoc.jenkins.io/component/stapler/org/kohsuke/stapler/DataBoundConstructor.html and optional parameter via DataBoundSetter

@RandomInEqualities
Copy link
Contributor Author

I have pushed a change that tries to use the DataBoundSetter, while also adding a test for FileTransformOperation. I have not tried to start a jenkins server and test it out. I have only run this project locally and run the tests, so I am not 100% sure if everything is ok with these changes.

@jonesbusy
Copy link
Contributor

The code looks good to me. I will try to perform some interactive tests in the next few days. Thanks

@jonesbusy
Copy link
Contributor

I think we need to be careful when adding new field

https://www.jenkins.io/doc/developer/persistence/backward-compatibility/#add-a-new-field

The default value must be true if not existent when the objects is resurected from storage (a job with legacy format loaded).

I think we can achieve it by

...
private Boolean useDefaultExcludes;
...

protected Object readResolve() {
  if (useDefaultExcludes== null) {
       useDefaultExcludes = true
  }
  return this;
}

The default value on the constructor can be kept when the object is created from code and not from storage.

I cannot confirm 100% of this, but I think it's the way to go

Copy link
Contributor

@jonesbusy jonesbusy left a comment

Choose a reason for hiding this comment

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

Conflict to resolve

Currently, it is not possible to copy or delete files
with the following names when using FileCopyOperation
and FileDeleteOperation:

CVS
.cvsignore
SCCS
vssver.scc
.svn
.DS_Store
.git
.gitattributes
.gitignore
.gitmodules
.hg
.hgignore
.hgsub
.hgsubstate
.hgtags
.bzr
.bzrignore

Since the glob Ant pattern that FilePath::list and
FilePath::copyRecursiveTo uses cannot pick up the files. This
change allows disabling the default exclude list when copying
transforming and deleting files. To allow e.g. deleting a
.gitignore file.

The Ant default exclude files are listed here:
https://ant.apache.org/manual/dirtasks.html#defaultexcludes
@RandomInEqualities
Copy link
Contributor Author

Year you were right, I added tests for the XStream serialization and indeed the readResolve was needed. It should be fixed now!

@jonesbusy jonesbusy merged commit 9d4e1eb into jenkinsci:main Jul 19, 2024
16 checks passed
@jonesbusy
Copy link
Contributor

Thanks for your PR, will be released in some minutes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants