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

Update as per origin #4

Open
tareiking opened this issue Mar 10, 2020 · 9 comments
Open

Update as per origin #4

tareiking opened this issue Mar 10, 2020 · 9 comments
Assignees

Comments

@tareiking
Copy link

@roborourke / @joehoyle - Is it possible to update our fork to include the latest changes upstream? I'm not familiar with the changes made or whether we are locking to this version in particular (vs. just haven't gotten around to updating our fork).

Want to submit tareiking#1 changes here and use our fork on a client project (vs. waiting for XWP upstream merge).

@joehoyle
Copy link
Member

Yeah TBH I don't actually know what patches we have included, so we should run a diff and see why we can't just switch over to upstream.

@tareiking tareiking mentioned this issue Mar 11, 2020
@tareiking
Copy link
Author

Apart from changing the name in composer.json, there isn't any actual patches against their develop branch.

For one of our internal projects, we would like to patch Stream vs. waiting for an upstream acceptance of said patch. However, that said - maybe its worth reaching out to add HM as contributors to the upstream patch and help with Stream overall (hattip: @ntwb for the alternative suggestion vs. forking)?

@tareiking
Copy link
Author

@joehoyle as an aside: Sync with upstream is always problematic. Do we have any auto-sync projects around or using something like https://github.com/wei/pull to automate this process?

@joehoyle
Copy link
Member

So, IIRC we needed this because XWP's version on packagist doesn't include all the assets built, but that might have changed now. @rmccue @roborourke could we re-check if we actually need to be maintaining our own fork to push to packagist?

@roborourke
Copy link

So it turns out we have a branch on this repo called build which Joe had been building the assets on and committing them, then tagging from there.

There is an upstream issue with their Travis config meaning tags are never built & deployed back to Github as intended. I opened a PR for them at any rate to fix it upstream: xwp#1055

It'd be preferable to just use the upstream repo if there are no other changes. Just double checking some code around network activation.

@roborourke
Copy link

Re. your comment here Joe #4 (comment) it looks like all the patches we've made are now in the upstream package.

@roborourke
Copy link

@tareiking you can PR your patch against this repo now. Once merged you can merge your changes to the build branch, run npm i && npm run build then commit the built files and make a new tag from that eg. 3.4.3. We'll switch Altis to use the upstream package once the travis PR is merged and a release deployed with that.

@kasparsd
Copy link

Noting here that we got the release builds added to the tags on GitHub xwp#1057 but they're not being picked up by Packagist. This is a known issue composer/packagist#903.

Yoast SEO solves this by having a dedicated distribution repository for just the artifacts (here is the deploy script). I've started working on it here xwp#1074 but the crazy private key restrictions on Travis for public repos is slowing everything down. I would strongly prefer we didn't have to do this crazy dance with encrypted SSH keys.

Is there a way for you to pull the plugin from the WordPress packagist instead? That contains just the built files.

@roborourke
Copy link

@kasparsd we're requiring it as a dependency in the altis/security package - dependencies can't declare custom repositories so we're stuck with packagist for that use case. Not sure we can force it to prefer dist at the subpackage level either.

Alternative is we continue to maintain this fork and push built versions but it's not ideal.

It's written for Circle CI but may be of interest - for some projects we build tags by checking for an update to the version string in the main plugin file, then commit the built files to a clean working tree and push that as a tag: https://github.com/humanmade/analytics/blob/master/.circleci/deploy.sh

Thanks for the updates so far and merging @tareiking's PR. I'll see if I can force it to use the zip somehow in the meantime.

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

No branches or pull requests

4 participants