umbraco / UmbPack

13 stars 13 forks source link

Add archive flag #28

Closed mattbrailsford closed 4 years ago

mattbrailsford commented 4 years ago

This PR adds a -a flag to UmbPack Push verb to archive packages on push

Supported values are "current" which is the default, which will archive whatever package is currently set to be the "Current", otherwise it's assumed the value is a simple wildcard pattern match such as My.Package.*.zip.

Fixes #18

mattbrailsford commented 4 years ago

Ok, so this assumes there will be an endpoint on Our at Umbraco/Api/ProjectUpload/ArchiveProjectFiles and this should accept a post request, with a JSON payload { ids: [123, 234, 456] } where the ID's are a list of package files to archive.

I think this also needs to check a current flag which I think #27 needs to implement in order to tell the push command that the push should be considered to be the current (right now it assumes any upload is current). Once that is implement, the behaviour of this could be updated such that the current archive behavior either always happens if the current flag is present, or at least requires current to be present for the "current" archive value to be allowed.

mattbrailsford commented 4 years ago

PS This is all untested as I'm not sure how you are testing these things? :joy:

jmayntzhusen commented 4 years ago

We test on a local version of Our which currently is not possible for you as the Our dev db had to be removed, a new one will come soon 🙂

We have a baseurl on UmbPack so if you build in debug mode it will send to a local Our instead: https://github.com/umbraco/UmbPack/blob/dev/src/Auth/AuthConstants.cs#L7

Will test this all out today and take a look at the Our endpoint as well 👍 Thanks a lot!

jmayntzhusen commented 4 years ago

Tested this, and other than my note above about the current not working until we make some changes, the archive stuff seems to work very well!

If I call something like this:

push C:\\pathhere\\UmbracoForms.Package.8.4.10.zip -c true -k [keyhere] -a UmbracoForms.Package.8.4.* UmbracoForms.Files.8.4.*"

It managed to find the correct packages out of a list, and returned all of their package ids 🎉

Also tested that -a * will work and just archive all other packages before pushing a new one as the current one 🙂

Next steps will be to create the Our endpoint and then I think I will merge and release it at that point, then a vNext iteration could be to fix the "current" mess on Our and then re enable your commented out current range 😅

mattbrailsford commented 4 years ago

In https://github.com/umbraco/OurUmbraco/issues/596 you say

instead we check the "current" package version by checking which file is set as the current one on the package node in the backoffice.

Does that not mean we couldn't do the same? Do we know from the package details we've pulled what the current "current" version is? so could we use that and check the file names instead of the current flag? Or if not, can we fetch it?

We could ultimately create an our endpoint and call it

var currentPackage = packageHelper.GetCurrentPackage(keyParts);

Then we could use that for now, and revert back to the boolean flag on the packages once that has been fixed.

jmayntzhusen commented 4 years ago

Does that not mean we couldn't do the same? Do we know from the package details we've pulled what the current "current" version is? so could we use that and check the file names instead of the current flag?

We'd have to do a contentservice call based on the package id first and get that value, but yes would be possible. I'll take a look after creating the Archive endpoint 🙂

jmayntzhusen commented 4 years ago

Archive endpoint PR: https://github.com/umbraco/OurUmbraco/pull/597

That + a few small changes in https://github.com/umbraco/UmbPack/pull/28/commits/8db9d56124ac378eab12f217d3782208dbfa3a0b & https://github.com/umbraco/UmbPack/pull/28/commits/bb9ac90c06ca347fc030c4f2e6d5c0328e3921db

means the functionality right now is working as mentioned above, so not the current (yet) but any space separated list of archive strings (including just -a * for all) 🎉

Only things left to do is create an endpoint for the current version temporarily until a larger Our change. Ran out of time for today though.

mattbrailsford commented 4 years ago

Awesome work @jmayntzhusen 🎉

jmayntzhusen commented 4 years ago

@mattbrailsford

I updated both PRs now with the new current package endpoint. One thought though - currently we set the "current" flag as true by default, and with the bit at https://github.com/umbraco/UmbPack/pull/28/files#diff-46441f50020ac69c83a779483482bf8bR92 it will actually not be possible to push a new current version without it automatically archiving the previous current.

It will remove the current if you set -a current packagename.1.2.* fx as well (+ whatever matches the 2nd string), but I'm thinking maybe there will be some cases where people want to set a new current without archiving the previous one? 🤔

mattbrailsford commented 4 years ago

@jmayntzhusen 🤔 hmm, if that is the case, then I think you'd have to say archiving can only occur by using the archive flag. We'd have to stop auto archiving anything and make it all explicit. I don't mind that personally as it makes it clear the intention and expected output. In which case, if the archive flag now works with a "current" option the this should all be ok.

It might confuse some if they are expecting that behavior based on previous releases, but I'd rather something "not happen" than something happening you didn't want to happen, if you see what I mean.

jmayntzhusen commented 4 years ago

Yep I removed it for now. So we still set current = true as default but can be overridden with -c false. The archiving will do nothing by default, but you can do either current for the current package or any string that may match packages 🙂

Thanks a lot for all the feedback and work on this 👍 Will merge soon and then get this + https://github.com/umbraco/UmbPack/pull/27 & https://github.com/umbraco/UmbPack/pull/31 out in a release 🙂

mattbrailsford commented 4 years ago

No problem. Looking forward to being able to use it, which I think this and the current update should make that possible. 🤘

mattbrailsford commented 4 years ago

PS See my thoughts on the inversion of the -c flag in issue #32 😁