Fix #8: Add public API

[danepowell]

Fix #8

Reference implementations:

• CLI-979: Consolidate update-check logic acquia/cli#1774
• Support selfupdatemanager self-update-fixture#4

[danepowell]

This works but I'm thinking we should actually cache the network request like in ACLI rather than just caching the response: https://github.com/acquia/cli/blob/main/src/Command/CommandBase.php#L1262

[danepowell]

@greg-1-anderson I need your input on how to handle caching.

I see Terminus has a hardcoded wait of 7 days between update checks: https://github.com/pantheon-systems/terminus/blob/3.x/src/Update/LatestRelease.php

ACLI on the other hand uses a Guzzle cache middleware which respects the max-age on the GitHub API response (typically 60 seconds): https://github.com/acquia/cli/blob/main/src/Command/CommandBase.php#L1262

In my ideal world, the actual time between update checks is ~24 hours. But I really like the idea of using a bog-standard HTTP cache in the self-update library rather than all consumers (ACLI, Terminus) having to hardcode their own update interval.

What would you prefer?

[danepowell]

I've decided to cache the GitHub response in the self-update library using symfony/cache; downstream projects can decide to implement polling periods however they'd like.

[greg-1-anderson]

A downstream library would need to construct the SelfUpdateManager with all the same parameters already passed to the SelfUpdateCommand

Regarding this question, I would recommend making a breaking change, and pass the SelfUpdateManager in to the constructor of the SelfUpdateCommand. The Front Controller would then create both of those objects, and the SelfUpdateManager can be cached / managed as needed for later automated use.

I don't have a strong opinion on caching. It would be good to have some level of configurability, with reasonable defaults provided.

[danepowell]

Thanks, I was just finishing this up as you commented. It's ready for review.

I took a similar approach, basically providing a method to get the manager from the command, to keep things backwards compatible. It feels a little dirty but I can't think of any practical reason not to do it.

Edit: actually, forcing downstream projects to build their own selfupdatemanager would make it easier to mock self-update checks without mocking the entire self-update command. I'm not sure if that's something you worry about in Terminus?

[greg-1-anderson]

I'd really prefer to break b/c for this, and make a new major version. I'll +1 this as-is if you really need b/c for some reason in one of your client projects.

[greg-1-anderson]

Also, you could mostly (maybe completely) preserve b/c if you made constructor parameters optional. Add SelfUpdateManager last, and instantiate one in the constructor if it's not provided. Some of the existing parameters of the constructor would have to become optional; the constructor should fail if a parameter that is subsumed by the self update manager is not null. Maybe that would be the best of both worlds? Then we could later make a new major version that removed the optional constructor parameters and made the self update manager required.

[danepowell]

I don't mind the BC break, I just want to do what's easiest, which is definitely not trying to support a bunch of optional arguments 😄

I refactored this to take the SelfUpdateManager as the only constructor parameter. It works pretty well, I've even got it working with autowiring in Acquia CLI so you don't have to actually hang onto the SelfUpdateManager in a client application. I still need to get tests working there, but I think it'll be fine.

Tests will pass here with consolidation/self-update-fixture#4

[danepowell]

@greg-1-anderson I'd appreciate a review this week if you're able. Tests should pass once we merge consolidation/self-update-fixture#4

[danepowell]

I'm going to merge this so we can close out the ongoing work and prevent merge conflicts, but I'll hold off on cutting a major release until I get your approval, and we can of course still make any changes you'd like.

Alpha (pre)release: https://github.com/consolidation/self-update/releases/tag/3.0.0-alpha1

[lyrixx]

Are we ready for the final 3.0 ?

[danepowell]

I'm fine with a 3.0.0 release but I want to give @greg-1-anderson a chance to approve (or at least veto 😄 ) it