feat: JENKINS-73837 - Adjusted DirScanner to allow empty directories in the TarArchiver

[DerSimeon]

See JENKINS-73837 .

Testing done

Proposed changelog entries

• Update the DirScanner to include empty Directories in the TarArchiver

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[DerSimeon]

It seems that a lot of Tests fail when presented with empty directories.

[MarkEWaite]

It seems that a lot of Tests fail when presented with empty directories.

Thanks for proposing the pull request. One of the expectations is that the automated tests must pass before a change is merged. We usually rely on the pull request submitter to make the necessary changes so that the existing automated tests are passing and are correctly expressing what is expected from the change in the pull request. Are you willing to make those changes?

[DerSimeon]

Thanks for proposing the pull request. One of the expectations is that the automated tests must pass before a change is merged. We usually rely on the pull request submitter to make the necessary changes so that the existing automated tests are passing and are correctly expressing what is expected from the change in the pull request. Are you willing to make those changes?

Yes absolutely, I am already working on the VirtualFileTests.
However some support in the DirectoryBrowserSupportTest would be appreciated as I am having some difficulty understanding what's going on there.

[DerSimeon]

@MarkEWaite could you take a look at my proposed fix?
While looking at the failing FilePathTest it appears as if it counts directories even if they aren't empty, causing FilePath#tar to return 4 (3 files + 1 dir).

[MarkEWaite]

@MarkEWaite could you take a look at my proposed fix? While looking at the failing FilePathTest it appears as if it counts directories even if they aren't empty, causing FilePath#tar to return 4 (3 files + 1 dir).

I think that test failure is another indication that your change is working as desired. Before your change, the contents of that file were:

-rw-rw-r-- 0/0            2049 2024-10-07 16:59 dir/pom.xml
-rw-rw-r-- 0/0            2049 2024-10-07 16:59 pom.xml
-rw-rw-r-- 0/0            1537 2024-10-07 16:59 small.tar

After your change, an entry is written to the tar archive for the directory dir. After your change, the contents of that file are:

-rw-rw-r-- 0/0            2049 2024-10-07 16:57 dir/pom.xml
-rw-rw-r-- 0/0            2049 2024-10-07 16:57 pom.xml
-rw-rw-r-- 0/0            1537 2024-10-07 16:57 small.tar
drwxrwxr-x 0/0               0 2024-10-07 16:57 dir/

I think it is correct to change the 3 in that test to a 4.

[DerSimeon]

@MarkEWaite hey! I have fixed all except one test. However with that last one I am going to need some support.

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[DerSimeon]

Also closes JENKINS-49296

[DerSimeon]

/reviewer @jenkinsci/core-pr-reviewers

[DerSimeon]

@timja could you check that failing windows test? to me it looks like a flaky test.

[timja]

@timja could you check that failing windows test? to me it looks like a flaky test.

re-triggered

[basil]

The new behavior has required a number of tests in core to be updated, so it is likely additional tests will need to be updated (ATH and/or plugins). Please run tests in jenkinsci/bom and jenkinsci/acceptance-test-harness with these bits to confirm no additional tests need adjustment.

I am also asking for security review of this PR, as it changes the expected behavior of a number of tests for past security fixes. The new expected behavior looks good to me, but I wanted the security team to confirm.

[basil]

As in commit 14cf0bf, please strengthen the assertion by maintaining the original containsInAnyOrder and adding any newly expected entries to the test.

[DerSimeon]

Will do today!

[DerSimeon]

@basil my latest commit should've fixed that. Could you double check?

[basil]

The code looks good to me. From my perspective, this PR is now ready for ATH/PCT testing and security review.

[basil]

Should add a containsInAnyOrder assertion after this to match the expected 6 entries, like the others in this file.

[DerSimeon]

The new behavior has required a number of tests in core to be updated, so it is likely additional tests will need to be updated (ATH and/or plugins). Please run tests in jenkinsci/bom and jenkinsci/acceptance-test-harness with these bits to confirm no additional tests need adjustment.

I am also asking for security review of this PR, as it changes the expected behavior of a number of tests for past security fixes. The new expected behavior looks good to me, but I wanted the security team to confirm.

Can you tell me how to run the ATH? do I just run mvn test in the test module?

[basil]

See jenkinsci/acceptance-test-harness#1748.

[DerSimeon]

so make a pull request and change the jenkins.version property if so, where do I get the value from?
Also once it passed all its test do I just close it and reference it here?

[basil]

When #9836 (comment) has been addressed and you have a passing CI build, the Checks tab will provide a section with the incremental build version number, which you can then consume in jenkinsci/bom and jenkinsci/acceptance-test-harness.

[DerSimeon]

Archiving the artifacts ** of a freestyle job with the shell step

mkdir -p foo bar bar/one bar/two
touch baz bar/qux bar/two/f

results in the directory foo not being archived, only the other empty dirs. This seems unexpected. Why archive subdirectories but not top-level directories?

A bunch of names and labels become (at least technically) wrong with this change, like the collectFiles method in this PR, or (download all files) and the help text of the "Archive the artifacts" post-build step on the UI. Additionally, the "Archive the artifacts" post-build step links to Ant documentation for the pattern definition, and I think Ant only supports files? So that would also be inconsistent if confirmed.

JENKINS-49296's wording suggests adding an option, while this does it unconditionally. This be considered a major change, as it will affect users relying on the current behavior for archived artifacts.

What would be your proposal? So far nobody has mentioned that, and from my tests the stated behaviour could not be verified. I will retry later when I have more time.

mistaken

[daniel-beck]

Thanks for challenging me on this, I checked again and I am unable to reproduce the behavior 🤦 No clue what I did, or thought I saw.

[daniel-beck]

However, I also don't see the intended behavior for JENKINS-49296, at least for the build-in "Archive the artifacts" freestyle post-build step: Downloading artifacts does not result in archives containing empty directories. Looking on the controller file system, empty directories are also not being archived there.

[DerSimeon]

Maybe empty directories are being additionally filtered somewhere?

[daniel-beck]

Maybe empty directories are being additionally filtered somewhere?

That would be my guess. I would appreciate if you could reproduce my findings independently first though, to ensure I didn't misinterpret something I was seeing again :)

If we don't have it yet (and didn't just fail for the above reason), the addition of a test going from CreateFileBuilder (or similar for dirs) to inspecting an artifact archive zip file, confirming contents, would be useful.