Add basic form to the upgrade flow #1218
Conversation
There was a problem hiding this comment.
Since DeploymentFormBody now supports the two different cases (a new deployment and upgrading), it can be considered as a "new" component (that's why this PR is so bulky).
It's a great change to DRY up the functionality between upgrade and install, but for the future, I think the bulk could be avoided and I could do a much better job reviewing this (ie. identifying between mechanical changes and actual reviewable changes), if it was 2 simple PRs as follows:
- Move of
DeploymentForm->DeploymentFormBodywith new parent DeploymentForm - this wouldn't require any mental effort to review as it's mostly a mechanical change, - Updating the UpgradeForm to use the
DeploymentFormBody
Thanks!
| defaultRequiredRBACRoles={{ update: this.requiredRBACRoles() }} | ||
| action="update" | ||
| resource={`Application ${releaseName}`} | ||
| /> |
There was a problem hiding this comment.
So this is why the apprepository rbac role was still required for upgrades? Or it wasn't? It's not clear why this is removed with this change.
There was a problem hiding this comment.
Yes, after your changes, it's no longer required to have RBAC permissions to upgrade an app here. We forgot to remove this then and while I was refactoring I removed that.
Yes, sorry for that, most of this work was figuring out what was needed and identifying what I could get out of the |
Ref: #1182
This PR adds the basic form to the
UpgradeFormcomponent. Before this PR,DeploymentFormandUpgradeFormwere independent components with a bunch of duplicated code. To avoid that, I have moved the common logic to a new componentDeploymentFormBodyso both components render that one and define its own particularities. This reduces the duplicated the code considerably.Since
DeploymentFormBodynow supports the two different cases (a new deployment and upgrading), it can be considered as a "new" component (that's why this PR is so bulky).