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

Alternative configurable deploy excludes #3

Merged
merged 3 commits into from
Aug 30, 2019
Merged

Alternative configurable deploy excludes #3

merged 3 commits into from
Aug 30, 2019

Conversation

LeoColomb
Copy link
Contributor

@LeoColomb LeoColomb commented Aug 29, 2019

Fix 10up/actions-wordpress#1, ref 10up/actions-wordpress#7 and can satisfy 10up/actions-wordpress#3

Replace 10up/actions-wordpress#14


This PR aims to let the source of files to deploy be easily configurable.

If GITHUB_TOKEN is provided, the process is exactly the same.
If not provided, we fallback to local files.
Using local files helps solving 10up/actions-wordpress#3, where we can use GitHub Actions with layers.

image

We use .distignore as suggested in #1 to ignore file with rsync while letting this be configurable.

The documentation may require an update, but let me know your thoughts on this first.

@helen
Copy link
Collaborator

helen commented Aug 30, 2019

Okay - it took me a while to understand what you were really solving for here, but I think I've got it. I noticed this while working on a plugin that has some built stuff in .gitignore which, of course, is also excluded by git archive.

With the help of @markjaquith, it looks like so long as it's in the right order --include-from can be used simultaneously so if people do have more complex include/exclude patterns we can handle that as well, maybe using something like .distinclude to be consistent (just detecting the presence of the file is fine, doesn't need a default). I don't know if we need to include that right now, though. Just wanted to make sure we thought that part through.

The actual ideal if wanting to re-use .distignore would actually probably be if @wp-cli parsed .distignore like .gitignore so you can have negation and then we could use that in place of .gitignore for git archive and not split the functionality. That said, it does seem like that requires more Git manipulation.

The thing I'm unsure about in your implementation for an initial merge is whether it's right to look for GITHUB_TOKEN as the flag for which method to use, or if it should look for .distignore first and prefer that if configuration is not set. Looking at the whole script now, I'm not even sure it really needs the GITHUB_TOKEN at all, I unfortunately don't seem to have documented why I originally added it and since the commit is never pushed I don't think it actually needs it.

@helen
Copy link
Collaborator

helen commented Aug 30, 2019

@LeoColomb I pushed a commit to remove GITHUB_TOKEN and to look for .distignore first before going back to .gitattributes. Going to merge this to develop and test it in a repo.

@helen helen merged commit c29ffc8 into 10up:develop Aug 30, 2019
@LeoColomb
Copy link
Contributor Author

I pushed a commit

Really nice, seems good to me! 👍

(sorry if my initial post was not clear, will try to explain better next time 😺)

@helen
Copy link
Collaborator

helen commented Aug 31, 2019

It worked! See https://github.com/10up/insert-special-characters/runs/208438277. I'm going to update the documentation for a bunch of things and also update the asset updater to use the same method, then merge to master and tag a new release.

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.

Plugin Deploy: Configurable excludes?
2 participants