Uploaded image for project: 'Atlassian Connect'
  1. Atlassian Connect
  2. AC-929

ACE should have an uninstalled lifecycle handler to clean up saved tenant settings

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Gliffy Diagrams

        Activity

        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: tpettersen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Tim Pettersen added a comment - - edited

        Perhaps uninstalling should just clean up the shared secret and add-on user? If we're going to clean up all data, we should ensure that the UPM uninstall message says as such, and make sure that we never invoke the uninstall lifecycle handler except through the user explicitly uninstalling the add-on. Upgrades, for example, should not cause the user to lose their data.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: tpettersen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Tim Pettersen added a comment - - edited Perhaps uninstalling should just clean up the shared secret and add-on user? If we're going to clean up all data, we should ensure that the UPM uninstall message says as such, and make sure that we never invoke the uninstall lifecycle handler except through the user explicitly uninstalling the add-on. Upgrades, for example, should not cause the user to lose their data.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: aholmgren; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Anders Holmgren added a comment -

        Sounds like this is impossible to tell.

        From Jonathan Doklovic on hipchat today

        it's impossible to have an "upgrade" callback... simply put, plug core doesn't have an upgrade event and it doesn't distinguish between disable/enable due to reinstall, dependency cycles, or upgrades.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: aholmgren; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Anders Holmgren added a comment - Sounds like this is impossible to tell. From Jonathan Doklovic on hipchat today it's impossible to have an "upgrade" callback... simply put, plug core doesn't have an upgrade event and it doesn't distinguish between disable/enable due to reinstall, dependency cycles, or upgrades.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: tpettersen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Tim Pettersen added a comment - - edited

        Then we may need to enhance the UPM to trigger an explicit "Uninstall" event when the user clicks the uninstall button. We should never delete user data unless the user explicitly requests it.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: tpettersen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Tim Pettersen added a comment - - edited Then we may need to enhance the UPM to trigger an explicit "Uninstall" event when the user clicks the uninstall button. We should never delete user data unless the user explicitly requests it.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: doklovic; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Doklovic added a comment -

        John Kodumal Ben Woskow this is another vote for a UPMUninstallHandler or a UPMUpgradeHandler... e.g. we want to do different things when we get a UPM uninstall request vs. a plug-core uninstall. Right now UPM just calls uninstall on the pluginController and let's it broadcast uninstall events which are the same during an upgrade.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: doklovic; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Doklovic added a comment - John Kodumal Ben Woskow this is another vote for a UPMUninstallHandler or a UPMUpgradeHandler... e.g. we want to do different things when we get a UPM uninstall request vs. a plug-core uninstall. Right now UPM just calls uninstall on the pluginController and let's it broadcast uninstall events which are the same during an upgrade.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: jnolen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Nolen added a comment -

        I would not advise doing this. People uninstall add-ons for all sorts of reasons, and through the history of our plugin system, that has NOT meant losing any data. We shouldn't violate that expectation. Deleting data should be a separate, deliberate step that is completely disconnected from any disable/uninstall process.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: jnolen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Nolen added a comment - I would not advise doing this. People uninstall add-ons for all sorts of reasons, and through the history of our plugin system, that has NOT meant losing any data. We shouldn't violate that expectation. Deleting data should be a separate, deliberate step that is completely disconnected from any disable/uninstall process.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: doklovic; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Doklovic added a comment -

        but this is a bit different.... if we uninstall a connect addon, we currently blow away:

        • the old descriptor we stored in plugin settings
        • the old base url we stored in plugin settings
        • the old shared secret we stored in plugin settings
        • the old connect addon user we stored in plugin settings
        • the old auth type we stored in plugin settings

        during install, we re-store these bits of data with fresh values. We need to do this in case anything changes (e.g. you go from oauth to jwt)

        I think we're saying that we could possibly handle things differently internall if we knew it was an upgrade up front.
        However, that's not the basis of this issue. THIS issue is saying that ACE should support the uninstalled lifecycle callback just like any BYOS json addon. This would give the addon the opportunity to clean up any of it's own data if it needs to (or do anything else it wanted to).

        If we think addons shouldn't blow away data that's one thing, but not supporting a documented feature in ACE is sucky.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: doklovic; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Doklovic added a comment - but this is a bit different.... if we uninstall a connect addon, we currently blow away: the old descriptor we stored in plugin settings the old base url we stored in plugin settings the old shared secret we stored in plugin settings the old connect addon user we stored in plugin settings the old auth type we stored in plugin settings during install, we re-store these bits of data with fresh values. We need to do this in case anything changes (e.g. you go from oauth to jwt) I think we're saying that we could possibly handle things differently internall if we knew it was an upgrade up front. However, that's not the basis of this issue. THIS issue is saying that ACE should support the uninstalled lifecycle callback just like any BYOS json addon. This would give the addon the opportunity to clean up any of it's own data if it needs to (or do anything else it wanted to). If we think addons shouldn't blow away data that's one thing, but not supporting a documented feature in ACE is sucky.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: jnolen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Nolen added a comment -

        Got it. OK – receiving the uninstall hook is fine. Automatically blowing away user data by default is not fine.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: jnolen; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Jonathan Nolen added a comment - Got it. OK – receiving the uninstall hook is fine. Automatically blowing away user data by default is not fine.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: sday; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Samuel Day added a comment -

        Just wanted to clear up what I was requesting with this ticket.

        Currently, when a product installs an addon built with ACE, the framework will call something along the lines of client.settings.set("clientInfo",

        { /* baseUrl / sharedSecret / etc */ }

        , clientKey). I consider this information to be something that ACE exclusively manages. That is, addons built with ACE should not be putting any additional data in `clientInfo`. I firmly believe that when the addon is uninstalled, ACE should transparently handle the `uninstalled` lifecycle event, and destroy the clientInfo data. As Jonathan Doklovic pointed out, all of this information is garbage after an uninstalled anyway, as the shared secret can change on re-install.

        Addons still need to be able to handle cleaning up any auxiliary data they store (whether it be in the ACE-provided JugglingDB storage, or elsewhere), but that is more a question of policy and how we document that for addon authors.

        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: sday; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Samuel Day added a comment - Just wanted to clear up what I was requesting with this ticket. Currently, when a product installs an addon built with ACE, the framework will call something along the lines of client.settings.set("clientInfo", { /* baseUrl / sharedSecret / etc */ } , clientKey). I consider this information to be something that ACE exclusively manages. That is, addons built with ACE should not be putting any additional data in `clientInfo`. I firmly believe that when the addon is uninstalled, ACE should transparently handle the `uninstalled` lifecycle event, and destroy the clientInfo data. As Jonathan Doklovic pointed out, all of this information is garbage after an uninstalled anyway, as the shared secret can change on re-install. Addons still need to be able to handle cleaning up any auxiliary data they store (whether it be in the ACE-provided JugglingDB storage, or elsewhere), but that is more a question of policy and how we document that for addon authors.
        Hide
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: pstreule; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Patrick Streule added a comment -

        Just blowing away the clientInfo entry on uninstall is perfectly fine IMO. Or more specifically, the default uninstall hook should do:

        Addon.settings.del('clientInfo', clientKey);
        

        but NOT:

        Addon.settings.del(clientKey);
        
        Show
        {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: pstreule; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Patrick Streule added a comment - Just blowing away the clientInfo entry on uninstall is perfectly fine IMO. Or more specifically, the default uninstall hook should do: Addon.settings.del('clientInfo', clientKey); but NOT: Addon.settings.del(clientKey);

          People

          • Assignee:
            {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: ; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Unassigned
            Reporter:
            {annotationCollection: [{}], annotations: [{}], collectionInheritableAnnotations: [{}], declaredAnnotations: [{}], description: Annotated value: sday; Annotations: [@com.atlassian.velocity.htmlsafe.HtmlSafe()]} Samuel Day
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Who's Looking?