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

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

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

claytoncollie
Copy link

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.

wp-cli/dist-archive-command#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:

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

@claytoncollie claytoncollie requested a review from a team as a code owner April 3, 2024 17:25
@claytoncollie claytoncollie requested review from peterwilsoncc and removed request for a team April 3, 2024 17:25
@claytoncollie claytoncollie self-assigned this Apr 3, 2024
@jeffpaul jeffpaul added this to the 2.3.0 milestone Apr 4, 2024
@jeffpaul jeffpaul requested a review from a team April 12, 2024 18:18
@jeffpaul jeffpaul requested a review from iamdharmesh May 13, 2024 17:34
@jeffpaul
Copy link
Member

jeffpaul commented Jul 8, 2024

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

faisal-alvi added a commit to faisal-alvi/action-wordpress-plugin-deploy that referenced this pull request Jul 22, 2024
@faisal-alvi faisal-alvi removed the request for review from a team July 22, 2024 15:07
faisal-alvi
faisal-alvi previously approved these changes Jul 22, 2024
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but I haven't tested them fully since the testing steps require actual deployment to WP Org.

@iamdharmesh
Copy link
Member

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.

@claytoncollie
Copy link
Author

@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.

@jeffpaul
Copy link
Member

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

@iamdharmesh
Copy link
Member

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.

@claytoncollie
Copy link
Author

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.

iamdharmesh/autoshare-for-twitter@cbc8267

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

@claytoncollie
Copy link
Author

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

@iamdharmesh
Copy link
Member

@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.

@claytoncollie
Copy link
Author

@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

@iamdharmesh
Copy link
Member

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:

@claytoncollie
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

WP CLI .distignore syntax is changing
4 participants