action-wordpress-plugin-deploy icon indicating copy to clipboard operation
action-wordpress-plugin-deploy copied to clipboard

Replace `--exclude-from` with `--filter` to allow for `.gitignore` syntax in `.distignore`

Open claytoncollie opened this issue 1 year ago • 15 comments

Description of the Change

Swap out the --exclude-from flag with the --filter flag to allow for the full .gitignore syntax to be used in the .distignore file. This change is to provide compatibility for breaking changes in the WP-CLI dist-archive command.

https://github.com/wp-cli/dist-archive-command/pull/78

I think this change might require a release by the dist-archive repository before being needed. At the time of opening this pull request, the last release was 2020 yet more items have been merged into main branch.

Closes #139

How to test the Change

  1. Create a .distignore to the plugin root
touch .distignore
  1. Create a text file named important.txt
touch important.txt
  1. Add a rule to the .distignore to ignore all .txt files but to include important.txt
echo -e '*.txt\n!important.txt' > .distignore
  1. Commit and deploy
  2. Verify important.txt is at the destination

Changelog Entry

Changed - replace --exclude-from with --filter when using .distignore to allow for full .gitignore syntax

Credits

Props @claytoncollie

Checklist:

  • [x] I agree to follow this project's Code of Conduct.
  • [x] I have updated the documentation accordingly.
  • [] I have added tests to cover my change.
  • [ ] All new and existing tests pass.

claytoncollie avatar Apr 03 '24 17:04 claytoncollie

@10up/open-source-practice would be good to get this through review if someone's free/able, thanks!

jeffpaul avatar Jul 08 '24 15:07 jeffpaul

Hi @claytoncollie,

Thanks for working on this and raising the PR. Apologies for the delay in reviewing it.

I tried to dry-run this PR action on https://github.com/iamdharmesh/autoshare-for-twitter repo (fork of 10up repo) and it's failing with rsync error: syntax or usage error (code 1) (Logs can be found here: https://github.com/iamdharmesh/autoshare-for-twitter/actions/runs/10111347459/job/27963100761). Could you please help to check if you are facing the same or it is the actual issue that needs to be fixed?

image

Thank you.

iamdharmesh avatar Jul 26 '24 13:07 iamdharmesh

@iamdharmesh Thanks for help testing this feature. I made one change to the --filter flag which now allows directory syntax that is used in the distignore file. The static analysis tool also caught an issue that I fixed on line 118.

claytoncollie avatar Aug 14 '24 14:08 claytoncollie

@iamdharmesh back to you for review/merge... thanks @claytoncollie!

jeffpaul avatar Aug 14 '24 16:08 jeffpaul

Hi @claytoncollie,

Apologies for the delay in reviewing again! Thanks for making the changes. The action is now passing without any issues for the existing .distignore files. However, I tried to test the full .gitignore syntax by following the test steps, but it doesn't seem to be working for me. Could you please help confirm if I missed anything?

I made changes to the .distignore file here: https://github.com/iamdharmesh/autoshare-for-twitter/blob/develop/.distignore to ignore all .txt files but include the readme.txt. However, readme.txt is not being included in the release zip(zip here). I'm not sure if I'm doing something wrong or missing anything. Could you please help check this?

Thanks again for all your help here.

iamdharmesh avatar Aug 26 '24 13:08 iamdharmesh

Hi @iamdharmesh, I looked at your autoshare repo and noticed that the .distignore was ignoring ALL *.txt files, which was overriding your !readme.txt rule. When I first looked, I noticed the README.md and CONTRIBUTING.md rules worked, which made me think there was a conflict for text files. Have a look at this commit and this release to see if the issue is resolved.

https://github.com/iamdharmesh/autoshare-for-twitter/commit/cbc82674d021f870b514d6d921d13cfe7cd6f2f8

https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.9

claytoncollie avatar Sep 16 '24 12:09 claytoncollie

@iamdharmesh I am re-reading your original comment about ignoring all text files and then having this single exclusion. Is that possible with .gitignore?

claytoncollie avatar Sep 16 '24 12:09 claytoncollie

@iamdharmesh I am re-reading your original comment about ignoring all text files and then having this single exclusion. Is that possible with .gitignore?

Hi @claytoncollie,

Thanks for checking on this. Yes, I am excluding all text files by default and then specifically including a single file. This can be done using both .gitignore and .distignore. When we run the wp dist-archive command, it will include that one text file while ignoring all other .txt files in the created zip.

iamdharmesh avatar Sep 16 '24 14:09 iamdharmesh

@iamdharmesh Can you test this again, please? I think this is the correct syntax.

https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.19

claytoncollie avatar Sep 16 '24 15:09 claytoncollie

Hi @claytoncollie,

Thanks a lot for working on this. It looks great now and resolves the issue I reported, Great work! While comparing the before-and-after behavior, I noticed that the new sync command does not exclude or delete files that are already committed to SVN, even if they are marked for exclusion.

For example, if a plugin repository has readme.txt and the /vendor directory is already committed to WP.ORG SVN, and we exclude them using .distignore by adding *.txt or readme.txt and /vendor, they are still not being excluded. Could you please look into this one? Please let me know if any further information is needed.

Thank you!

Some links related to my test:

  • .distignore file: https://github.com/iamdharmesh/autoshare-for-twitter/blob/d787cc02979463e61b598810f322ed0cc07b339e/.distignore
  • Expected zip: https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.22
  • Zip generated with this PR: https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.21

iamdharmesh avatar Sep 17 '24 15:09 iamdharmesh

Hi @iamdharmesh I will have to pick this up next week.

claytoncollie avatar Sep 18 '24 20:09 claytoncollie

Hi @iamdharmesh I will have to pick this up next week.

Hi @claytoncollie have you had the chance to look into this?

FawadNL avatar Feb 09 '25 20:02 FawadNL

Hi @iamdharmesh

I spent some time on this, and what I figured out is that the --filter property of rsync only accepts files with rules following the rsync syntax; files with rules following the .gitignore syntax wouldn't work.

What I did was to follow an alternative approach. I chose to convert what's inside the $GITHUB_WORKSPACE into a reinitialized Git repository with the previous .distignore file acting as the new .gitignore file. This way, I'm building a list of files to be included, and then I'm using this list to perform the rsync.

Even though this is working fine with new files and folders, I had trouble excluding files and folders that are already part of the SVN repo, but that should be an existing problem. Do you think this needs to be fixed in the scope of this task?

Thank you

kmgalanakis avatar May 29 '25 06:05 kmgalanakis

Thanks for checking on this and trying a different approach, @kmgalanakis.

Even though this is working fine with new files and folders, I had trouble excluding files and folders that are already part of the SVN repo, but that should be an existing problem. Do you think this needs to be fixed in the scope of this task? but that should be an existing problem.

The current stable version doesn't have any issues with excluding files, so I just wanted to understand this better. Could you please provide more information on it?

Thanks!

iamdharmesh avatar May 30 '25 17:05 iamdharmesh

@kmgalanakis any updates from your end based on Dharmesh's comment above?

jeffpaul avatar Jul 07 '25 18:07 jeffpaul