Allow user to rename app if buildpack is deleted #2321
Conversation
…RenameApplicationByNameAndSpaceGuid to use new UpdateApplicationName function.
- Use annonymous stuct for requestBody that only contains Name since the goal is to only have a name field - should we just go declare a real resuable struct or this truely a one off thing? - Ran go generate ./... to get correct fake_cloud_controller_client - go generate ./... also made lots of other updated fakes. Removed them for this feature but maybe we should make a pr just running a newer counterfeiter? - Update tests to make sure everything is correctly nested - Update tests to account for annoynmous struct - next step is to run integration tests
- when calling `cf rename` on the previous commit, it would error stating "Unknown field Name" - this was because the field is really name, but wasn't being marshalled due to annoymous struct - create ApplicationNameOnly struct that marshalls correctly - another question - should there be an integration test for this?
moleske
requested review from
PeteLevineA,
a-b,
pivotalgeorge,
reedr3,
jdgonzaleza and
ccjaimes
|
There's a few questions in the commit messages, but the one I'm most concerned for is should there be an integration test added. Current Integration tests all passed on a cf-deployment with capi 1.137.0, though I didn't enable some experimental features like route_sharing so didn't test those |
|
|
||
| updatedApp, warnings, err := actor.CloudControllerClient.UpdateApplicationName(newAppName, appGUID) | ||
| if err != nil { | ||
| return resources.Application{}, Warnings(warnings), err |
There was a problem hiding this comment.
Any reason why we are returning resources.Application{} instead of nil?
There was a problem hiding this comment.
seems like the norm here... :)
@moleske Our pipelines have experimental features such as route_sharings already enabled :). |
@PeteLevineA and I thought about this. We think it is not changing the expectation of the current clients. This is because |
* Add new UpdateApplicationName functions for actor and client. Change RenameApplicationByNameAndSpaceGuid to use new UpdateApplicationName function. * Add UpdateApplicationName to cloud-controller-client interface. * create ApplicationNameOnly struct that marshalls correctly Co-authored-by: Ryker Reed <[email protected]>
* Add new UpdateApplicationName functions for actor and client. Change RenameApplicationByNameAndSpaceGuid to use new UpdateApplicationName function. * Add UpdateApplicationName to cloud-controller-client interface. * create ApplicationNameOnly struct that marshalls correctly Co-authored-by: Ryker Reed <[email protected]>
cc @reedr3
Thank you for contributing to the CF CLI! Please read the following:
If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
Does this PR modify CLI v6, CLI v7, or CLI v8?
Going to default branch, probably will request a backport to v8
Please see the contribution doc above or review Architecture Guide.
Description of the Change
A user had an app that they wanted to rename, but the buildpack the app was using had previously been deleted from the cloud foundry deployment. When the user called
cf renamethey received an error about the buildpack not being available. CAPI supports only renaming an app, but the cf cli is passing not just the rename oncf renamebut the stack and buildpack information. This pr allows only rename by only passing to capi the new name information and not the stack and buildpack information.We must be able to understand the design of your change from this description.
Keep in mind that the maintainer reviewing this PR may not be familiar with or
have worked with the code here recently, so please walk us through the concepts.
Why Is This PR Valuable?
What benefits will be realized by the code change? What users would want this change? What user need is this change addressing?
CAPI allows the user to only do renames via its API so we should also support this
Why Should This Be In Core?
Explain why this functionality should be in the cf CLI, as opposed to a plugin.
Not new functionality, just fixing existing functionality.
Applicable Issues
List any applicable Github Issues here
How Urgent Is The Change?
Is the change urgent? If so, explain why it is time-sensitive.
Other Relevant Parties
Who else is affected by the change?