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

Integration of afragen/git updater lite #317

Merged
merged 9 commits into from
Feb 25, 2025

Conversation

afragen
Copy link
Contributor

@afragen afragen commented Feb 23, 2025

Pull Request

Integrate Git Updater Lite

(Please list the changes you made...)

  • Add composer require for afragen/git-updater-lite: ^2
  • Add initialization command to main plugin file
  • Added Updater URI header to main plugin file but will need to change the Update URI header once an AspirePress Update API Server is set up.

Did you fix any specific items

Simpler update and integration of AspireUpdate plugin than exists in AC
Fixes #314
Fixes #240

CERTIFICATION

By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.

This integration will work with any plugins/themes that have data in the Update URI header, only requirement is AspireUpdate be active.

There is an update to GUL coming but the code is all in here, it's just incorrectly versioned so don't run composer update until GUL v2.2.0 is released.
@afragen afragen requested review from chuckadams, asirota, toderash, namithj and a team February 23, 2025 19:23
@afragen
Copy link
Contributor Author

afragen commented Feb 23, 2025

PR will fail testing until I commit GUL v2.2.0. Awaiting test updates for it.

@namithj
Copy link
Contributor

namithj commented Feb 23, 2025

Why do we need this in AU?
If its just for Git Updater functionality, lets keep the Git Updater header but keep the Git Updater plugin separate and for those who install it. I don't see a need to ship Git Updater with AU considering its already available as a separate plugin for users.

Please dont merge this in until the requirement is discussed and everyone is onboard.

@chuckadams
Copy link
Contributor

Does this require connection with a GU server instance, or is this just using the GUL routines to process the downloaded zip? And does fixing the zip fix the issue with non-detection? I thought that was related to the dash in the dirname.

@namithj
Copy link
Contributor

namithj commented Feb 23, 2025

Does this require connection with a GU server instance, or is this just using the GUL routines to process the downloaded zip? And does fixing the zip fix the issue with non-detection? I thought that was related to the dash in the dirname.

Thats another PR #315

@afragen
Copy link
Contributor Author

afragen commented Feb 23, 2025

It does require a connection with a GU Update API Server instance.

It will use GUL to update AU.

@afragen
Copy link
Contributor Author

afragen commented Feb 23, 2025

Fixing the workflow is different.

@afragen
Copy link
Contributor Author

afragen commented Feb 23, 2025

The only similar function between GU and GUL is updating and the data for the View details modal.

GU has so much more and most users won't have GU installed, it's a developer tool.

@afragen
Copy link
Contributor Author

afragen commented Feb 24, 2025

FWIW I took out the class GUL_Loader and simply added it for AU.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

Some info on why this is beneficial:

Currently, AspireUpdate is only available from AspireCloud or direct download. Git Updater Lite decouples updating AspireUpdate from AspireCloud, so that AspireUpdate will receive updates even if the selected server goes down (whether AC production, AC bleeding edge, third-party instance of AC, or third-party API).

Integrating Git Updater Lite means it retrieves the latest update data from the AspirePress website, and pulls it from GitHub. This saves us having to upload a new asset ZIP and maintain update data, which would be required otherwise.

@namithj
Copy link
Contributor

namithj commented Feb 24, 2025

If we ourself are not confident about our update system that we need to fall back on another system, how would we convince others to use AC. This is an Ask.com tool bar like situation where we Trojan horse in another updater. If AC has limitations we should work to adddress that and not try and bypass it.

@namithj
Copy link
Contributor

namithj commented Feb 24, 2025

Additionally we already spoke about using the GU api which @afragen is developing as another source in AE (which will then reach AU eventually). Unless a user chooses GU as the plugin source we should not push it upon them. Let's keep AC and GU separate.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

If we ourself are not confident about our update system that we need to fall back on another system, how would we convince others to use AC.

It's not just about confidence. The fundamental purpose of AspireUpdate is to rewrite API urls to another location. That location may not have AspireUpdate in its listings. We therefore need to ensure AspireUpdate is decoupled from the side effect of its own actions.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

Additionally we already spoke about using the GU api which @afragen is developing as another source in AE (which will then reach AU eventually). Unless a user chooses GU as the plugin source we should not push it upon them. Let's keep AC and GU separate.

This isn't merging AspireCloud and Git Updater. It's integrating Git Updater Lite and AspireUpdate specifically to handle updating to AspireUpdate from its canonical source because the configured API host may not have any AspireUpdate source available.

While AspirePress would of course like to see everyone using AspireCloud, AspireUpdate, and AspireExplorer in conjunction, there's no requirement to use them with each other. Therefore, what we do in one project shouldn't be assumed to benefit another.

@namithj
Copy link
Contributor

namithj commented Feb 24, 2025

We should respect the users decision in that scenario, if we really want to decouple AU from the update process, the functionality should be added to AU core and not as a dependency.

We should not be introducing third party projects into AU core unless it's mission critical as it will impact security and stability as it's beyond our control.

Dependency decisions like this need to have a triage and then only moved to code phase.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

We should respect the users decision in that scenario

"the user's decision" in this scenario may have one of these outcomes:

  1. Switched to an API server that doesn't have AspireUpdate available and therefore AspireUpdate can only be updated manually. The user is less likely to know that an update is available, or to manually upgrade.
  2. Switched to an API server that may serve its own copy of AspireUpdate, which could have changes in it. This is a supply chain vulnerability.

We don't have control over those third-party API servers - that's the security and stability issue.

We should not be introducing third party projects into AU core unless it's mission critical as it will impact security and stability as it's beyond our control.

Of course it's within our control. We choose what we ship with AspireUpdate. If we've tested a newer version of a dependency and it's not stable or secure, we can lock it to the previous version. I'd also say this is mission critical.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

This is an Ask.com tool bar like situation where we Trojan horse in another updater.

Also, I'd ask that you take a moment to reconsider before posting this kind of thing. It's needlessly dramatic and taints a discussion by presenting one view as being dishonest or even malicious.

@namithj
Copy link
Contributor

namithj commented Feb 24, 2025

  • Switched to an API server that doesn't have AspireUpdate available and therefore AspireUpdate can only be updated manually. The user is less likely to know that an update is available, or to manually upgrade.

  • Switched to an API server that may serve its own copy of AspireUpdate, which could have changes in it. This is a supply chain vulnerability.

Both are valid user decisions and we should respect that. I really don't understand why we have to bypass everything and auto update one plugin. AU should be considered as another plugin, it should have no special privileges.

@costdev
Copy link
Contributor

costdev commented Feb 24, 2025

Both are valid user decisions and we should respect that.

A user most likely won't know that their choice doesn't have AspireUpdate or has its own source for AspireUpdate. Their decision was to choose an API host. That doesn't mean the host has informed them, nor that the user will have considered that risk.

I really don't understand why we have to bypass everything and auto update one plugin. AU should be considered as another plugin, it should have no special privileges.

This is no different to using the UpdateURI header for what it was designed for, and we're using a dependency that handles all the aspects we'd need to cover instead of reinventing the wheel. The dependency is written by a trusted member of AspirePress, who has contributed to AspireUpdate, and has 10+ years knowledge the underlying systems, including as a maintainer of the Core component. It's not a case of grabbing a random library.

This doesn't auto-update the plugin. WP Core won't auto-update the plugin unless the user has enabled auto-updates for it.

@afragen afragen marked this pull request as ready for review February 24, 2025 17:08
@namithj namithj merged commit b2c2841 into aspirepress:main Feb 25, 2025
2 of 6 checks passed
@toderash
Copy link

Just to complete the loop here for documentation, this was decided in our weekly PM meeting after reviewing all the concerns and the proposed PR. The approach of updating AspireUpdate from GitHub rather than from AspireCloud closes a potential security issue in the supply chain. This doesn't need documenting here, but there are specific reasons to handle this outside of ACloud, and the concerns expressed have been identified and addressed accordingly. cc @costdev

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.

[Bug]: AspireUpdate plugin not detected on plugins page Plugin should autoupdate
5 participants