Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

TEAMFOUR-804: Endpoint Dashboard: Delete/Unmap Route #519

Merged
merged 5 commits into from
Aug 4, 2016

Conversation

richard-cox
Copy link
Contributor

Initial commit for delete/unmap route action items in endpoints space routes tab

@richard-cox
Copy link
Contributor Author

@ongk If you have some spare time today could you quickly review some parts of this PR? I've added some framework that am not sure is the best way forward. Before continuing would love to get your feedback.
Basically trying to share some common code between the endpoints dashboard and the application summary page (delete/unmap routes). At the moment I've created a common service (src/plugins/cloud-foundry/service/routes.service.js) that is registered in a newly created serviceManager (just like modelManager). This is then 'retrieved' both places. The reason I'm hesitant is due to serviceManager being new. Is this use case tackled in another way elsewhere?

@ongk
Copy link
Contributor

ongk commented Aug 3, 2016

Hmm, I'm not sure we need a serviceManager since those can just be injected wherever you need to use it.

richard-cox added 2 commits August 3, 2016 11:38
Conflicts:
	src/app/view/endpoints/clusters/cluster/cluster.module.js
@richard-cox
Copy link
Contributor Author

@ongk Ahh, ok, thanks. Have made the change and updated. @julbra Should be good to go :)

return this.apiManager.retrieve('cloud-foundry.api.Spaces')
.ListAllRoutesForSpace(guid, options, this.makeHttpConfig(cnsiGuid))
.ListAllRoutesForSpace(guid, params, this.makeHttpConfig(cnsiGuid), params)
Copy link
Contributor

@julbra julbra Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the second params should be options? Or is it one too many arguments?

@julbra julbra merged commit acca964 into master Aug 4, 2016
@julbra julbra deleted the 804-delete-unmap-route branch August 4, 2016 12:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants