umbraco / UmbPack

13 stars 13 forks source link

Should push update the 'current version' on Our? #17

Closed LottePitcher closed 4 years ago

LottePitcher commented 4 years ago

I pushed a package to Our. The command successfully extracted the version as 1.4 from the package.xml:

image

Is it correct that on Our my package is still showing as version 1.0?

image

This push was setting this file as the current version (the default behaviour of 'push'), so I was kinda expecting the current version to be updated to match the version in package.xml. Should that have happened?

mattbrailsford commented 4 years ago

I haven't tried UmbPack yet but this was one of my initial thoughts as to whether this value gets updated. I agree if the package is made "current" then the current version should also be updated.

jmayntzhusen commented 4 years ago

Ah good reporting, this is an oversight I think 🙂

Will have to be fixed here: https://github.com/umbraco/OurUmbraco/blob/master/OurUmbraco/Project/Api/ProjectUploadController.cs#L97

Will look into it next week.

jmayntzhusen commented 4 years ago

Had a quick look at this today, and what a mess! 🤯

The problem is...

We are currently "storing" the version of a package 3 seperate places:

  1. On the content node of the package in the backoffice, that is the one you can update on the edit step, and for some reason that is not on the edit files step even though that is where you would upload a new version and set it to current (historically, before UmbPack atleast)
  2. We postfix the version in the name of the zipfile, this is convention and something that is done automatically with UmbPack, but not sure that is something we can rely on always being the case? Ex. packagename_1.0.0.zip
  3. In the package.xml we have an xml element:
    <umbPackage>
    <info>
    <package>      
      <version>1.0.0</version>

I think the best way forward would be the following:

  1. Get rid of the property value for the current version and instead set the version based on the version of the current package file - Have to investigate where this is used before getting rid of it completely (may be used in the version range of the pkg as well).
  2. Add some validation on package upload where we enforce that a package is named {pkg name}_{semver version}.zip - this would need to be both on manual and UmbPack uploads.
  3. Ensure that when we run the umbpack pack command we update the package.xml version to the specified version so the version in there and the version in the name of the zip are in sync.

Had hoped for a quick and easy fix for this one, but it is a bit more complicated than at first sight 😬

Any thoughts / ideas / comments on this?

mattbrailsford commented 4 years ago

Personally, I think you are going to have to get the current version from the current packages package.xml file, in the version field. I think basing it on the filename would be too error prone as I know not everyone follow this standard and to enforce it now, could be problematic.

I'd agree that we probably don't need the "current version" field if we can read it from the package.xml file of the current package.

jmayntzhusen commented 4 years ago

Hmm reading it from the package.xml will instead force people to keep that up to date 😁 But yea it's probably a better solution, and I guess that is how the CMS backoffice also determines versions of packages..

Alright will create proper issues for the tasks needed on Our and UmbPack tomorrow then 🙂

mattbrailsford commented 4 years ago

I think it's safer to enforce that field in the package.xml file as that is what that is for vs the filename. I think it also ensures that the package is somewhat valid too.

mattbrailsford commented 4 years ago

Hey folks, just wondering if there has been any updates on this? Would love to start integrating UmbPack in my build process, but I think this and one other are holding me back atm.

jmayntzhusen commented 4 years ago

Hey Matt,

Unfortunately the Our dev db had to be revoked recently, and we (HQ) are working on getting a new one out so PRs to Our can be made and tested again. Until that happens we (package team) unfortunately can't do much work that requires Our like this issue.

I will update this issue whenever there are news 🙂

mattbrailsford commented 4 years ago

Thanks for the update @jmayntzhusen 👍🏻

jmayntzhusen commented 4 years ago

PRs: https://github.com/umbraco/OurUmbraco/pull/590 https://github.com/umbraco/UmbPack/pull/27

Will aim to get it out by end of the week @mattbrailsford 🙂

mattbrailsford commented 4 years ago

Wh00p wh00p! 🎉

Nicely done 👏👏👏