Bug fix/curb aardvark foxx cthulhu #9867
Conversation
This reverts commit 6092465.
| Flag to run setup after install. | ||
| `) | ||
| .queryParam('teardown', joi.boolean().default(false), dd` | ||
| Flag to run teardown before replace/upgrade. |
There was a problem hiding this comment.
These apparently weren't actually implemented in aardvark before.
| } | ||
| const $replace = $('#new-app-flag-replace')[0]; | ||
| if ($replace) { | ||
| flags.replace = Boolean($replace.checked); |
There was a problem hiding this comment.
The DOM APIs have been stable for years, this even works in IE now.
|
|
||
| if ($('#new-app-replace').prop('checked')) { | ||
| flag = true; |
There was a problem hiding this comment.
Previously the flag was apparently some kind of weird enum. We're now using an object with actual boolean flags.
| tableContent.push( | ||
| window.modalView.createCheckboxEntry( | ||
| 'new-app-teardown', | ||
| 'new-app-flag-setup', |
There was a problem hiding this comment.
Seems cleaner to prefix these consistently with "-flag"
| 'Run setup?', | ||
| true, | ||
| "Should this app's setup script be executed after installing the app?", | ||
| "Should this service's setup script be executed after installing the service?", |
There was a problem hiding this comment.
This is just for consistency. We renamed "applications" to "services" in 3.0.
| 'new-app-flag-teardown', | ||
| 'Run teardown?', | ||
| true, | ||
| "Should the existing service's teardown script be executed before replacing the service?", |
There was a problem hiding this comment.
We didn't actually ask about the teardown when replacing an app. This used to be implied by the replace/upgrade flag.
| installFromGithub: function (info, mount, callback, isLegacy, flag) { | ||
| var url = arangoHelper.databaseUrl('/_admin/aardvark/foxxes/git?mount=' + encodeURIComponent(mount)); | ||
| if (isLegacy) { | ||
| install: function (mode, info, mount, options, callback) { |
There was a problem hiding this comment.
These methods all did the same thing except using slightly different URLs. We now pass the bit in the URL that changes in the arguments as "mode".
| if (options.replace === true) { | ||
| url += '&replace=true'; | ||
| } else if (options.replace === false) { | ||
| url += '&upgrade=true'; |
There was a problem hiding this comment.
This previously used to be the magic enum flag.
| } else { | ||
| mount = window.arangoHelper.escapeHtml($('#new-app-mount').val()); | ||
| if (mount.charAt(0) !== '/') { | ||
| mount = '/' + mount; | ||
| } | ||
| } | ||
| if (flag !== undefined) { | ||
| this.collection.installFromStore({name: this.toInstall, version: this.version}, mount, this.installCallback.bind(this), flag); | ||
| } else { |
There was a problem hiding this comment.
These two are the same except once without the argument if it's undefined. That's silly because the function doesn't check the argument count and undefined is undefined.
| if (version === '') { | ||
| version = 'master'; | ||
| } | ||
| var info = { | ||
| url: window.arangoHelper.escapeHtml($('#repository').val()), | ||
| version: window.arangoHelper.escapeHtml($('#tag').val()) |
There was a problem hiding this comment.
This is literally the same code as a few lines earlier except it ignores the default for "master". This is likely a bug.
| this.collection.installFromZip(window.foxxData.data.filename, mount, this.installCallback.bind(this), isLegacy, flag); | ||
|
|
||
| info = { | ||
| zipFile: window.foxxData.data.filename |
There was a problem hiding this comment.
This wrapping previously happened in installFromZip.
| } else { | ||
| mount = window.arangoHelper.escapeHtml($('#new-app-mount').val()); | ||
| if (mount.charAt(0) !== '/') { | ||
| mount = '/' + mount; | ||
| } | ||
| } | ||
| url = window.arangoHelper.escapeHtml($('#repository').val()); | ||
| var info = { | ||
| url: url, | ||
| version: 'master' |
There was a problem hiding this comment.
The version attribute does nothing.
|
These are all untested and I haven't built the frontend yet to check if they actually work. @hkernbach This PR is based on your PR so merging will merge it into your PR, not devel. |
* Revert "fixed ui behaviour when replacing a foxx app" This reverts commit 6092465. * Cleanup on aisle 3 * Typo * Fixed param order * Cleanup flags * Actually use the queryParams
* Revert "fixed ui behaviour when replacing a foxx app" This reverts commit 6092465. * Cleanup on aisle 3 * Typo * Fixed param order * Cleanup flags * Actually use the queryParams
* fixed ui behaviour when replacing a foxx app * Bug fix/curb aardvark foxx cthulhu (#9867) * Revert "fixed ui behaviour when replacing a foxx app" This reverts commit 6092465. * Cleanup on aisle 3 * Typo * Fixed param order * Cleanup flags * Actually use the queryParams * currently not supported via grunt env * changelog * removed not needed view * fixed wrong used function
* release version 3.4.8 * [3.4] fix agency lockup when removing 404-ed callbacks (#9839) * fixed ui behaviour when replacing a foxx app * Bug fix/curb aardvark foxx cthulhu (#9867) * Revert "fixed ui behaviour when replacing a foxx app" This reverts commit 6092465. * Cleanup on aisle 3 * Typo * Fixed param order * Cleanup flags * Actually use the queryParams * currently not supported via grunt env * changelog * removed not needed view * fixed wrong used function
This is an attempt to fix the underlying issue in #9719.