Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reuse unbound routes #1223

Merged
merged 17 commits into from
Aug 18, 2017
Merged

Reuse unbound routes #1223

merged 17 commits into from
Aug 18, 2017

Conversation

KlapTrap
Copy link
Contributor

  • Allow users to attach already create routes within the applications space to an applicaiton.
  • Will only list routes that are not already attached to the application.
  • Fixed tests.
  • Added test.
  • Couldn't run e2e tests on my mac so going to cheekily run them via concourse.

KlapTrap added 16 commits August 15, 2017 10:45
@KlapTrap
Add tabs to add route modal
fc20731
@KlapTrap
Add getUnboundRoute method to route model and small tidy ups
bf7ab40
@KlapTrap
List existing routes on add route modal
dd49d12
@KlapTrap
Tidy up route fetch code
c1c6142
@KlapTrap
Check validilty of add route modal depending on what tab is selected
3848e51
@KlapTrap
Allow users to assign existing route to application
266e76e
@KlapTrap
Fix undefined form validation exception
aaffa29
@KlapTrap
Actually use the st-table directive and filtering
8db1280
@KlapTrap
Update resue route tab styling
9d44456
@KlapTrap
Don't show existing routes tab if no existing routes can be found
e241aef
@KlapTrap
More styling changes for the existing routes tab
536b2a4
@KlapTrap
Override nav styles on route modal
de2f3fc
@KlapTrap
Added 'load all' option to route table
fda03ad
@KlapTrap
Remove uneeded application id from routes request
0a40c93
@KlapTrap
Fixed and added test for route add modal
26bb49c
@KlapTrap
Merge branch 'master' into reuse-unbound-routes
c10498a 
* master:
  Fix style issue
  Persist app wall selection of cf/org/space in local storage (#1214)
  Add upgrade documentation for helm repository based installation (#1218)
  Fixed docker image name for all-in-one deployment (#1216)
  Use stable name for docker registry (#1220)
  Update CI piplines for environment (#1217)
  Fixes and changed select source radio button to select-input dropdown - Converted the select source radio button to a drop down (ready for more sources) - Split out more styles from deploy-step-source.scss - Fixed github userinput validation - Fixed display of cf ignore info for manually opened file/folders
  Deploy fixes, removed common session data, added delay to 'scanning' busy screen
  Started deploy step, a few minor bugs to work through WIP
  UX for source mostly works now. Still plenty of tidy up to do WIP
  Refactor/reduce deploy app service WIP - Move steps out into their own service - Move input sources out into seperate directives
  Split deploy destination + source step into two steps
richard-cox
richard-cox suggested changes Aug 18, 2017
View reviewed changes
Copy link
Contributor

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Looks good. There's a few minor points though (see inline).
Additionally if you add enough routes to show the 'show more' option and shrink the modal the existing route table shows a vertical scroll bar prematurely.

components/cloud-foundry/frontend/src/view/applications/application/summary/summary.scss Outdated
width: auto;
}
.control-label {
margin-left: -15px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is needed? Would be a departure from the style of all other input fields

components/app-core/frontend/src/view/navbar/navbar.scss Outdated
@@ -222,12 +222,11 @@ nav.secondary-nav {

&:hover {
color: $navbar-hover-fg;
background-color: $navbar-hover-bg;
Copy link
Contributor

Choose a reason for hiding this comment

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

These two background-color's are needed

components/cloud-foundry/frontend/src/api/Apps.js
@@ -7,6 +7,8 @@
.module('cloud-foundry.api')
.run(registerApi);

var BASE_URL = '/pp/v1/proxy/v2/apps';
Copy link
Contributor

Choose a reason for hiding this comment

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

In the past we've avoided making changes to 'api' files as they were originally auto-generated. Going forward I don't think there's much harm in re-factoring but we need to bear in mind we might auto-generate these files for v3 of the cf api.

components/cloud-foundry/frontend/src/model/route.model.js
@@ -25,6 +25,8 @@
*/
function Route(apiManager, modelUtils) {

var routesApi = apiManager.retrieve('cloud-foundry.api.Routes');
Copy link
Contributor

Choose a reason for hiding this comment

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

We've had some issues in the past where the apiManager/modelManager cannot retrieve the objects when their host function is run first, so will grab them when needed. It's probably best to leave these as is.

...ud-foundry/frontend/src/view/applications/application/summary/add-route/add-route.service.js Outdated
* @constructor
*/
function AddRouteServiceFactory(modelManager, frameworkAsyncTaskDialog) {
function AddRouteServiceFactory(modelManager, frameworkAsyncTaskDialog, $q, appClusterRoutesService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As a general rule we try to put angular $ services first. This practise wasn't applied from start though so you may still find places where they appear anywhere

...d-foundry/frontend/test/unit/view/applications/application/summary/add-route.service.spec.js
spaceGuid +
'/routes?include-relations=domain,apps&inline-relations-depth=1&results-per-page=100';

var expectedRoutesRes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an old practise of placing large responses in *.mock.spec.js files. Think the original plan was to build up a library of calls. Here's it's ok but if there's multiple it may be best to move to route.model.spec.js

@richard-cox richard-cox merged commit c92c19a into master Aug 18, 2017
@richard-cox richard-cox deleted the reuse-unbound-routes branch August 18, 2017 16:20
KlapTrap added a commit that referenced this pull request Aug 21, 2017
@KlapTrap
Merge branch 'master' into update-npm-dpes
65ca399 
* master:
  Reuse unbound routes (#1223)
  Revert "Added ulimit note to development readme"
  Show services in app summary page, auto pick service plan in add service modal
  Changes following review
  If the value of a 'select-input' is a placeholder always translate
  Fix e2e
  Moved logic into specific cloud-foundry-hosting plugin
  Final bits n bobs - Switch logic from 'disable' flag to 'enable' (true by default) - Updated docs - Prooved cf push with/wuthout flag
  Fix e2e
  Disable the endpoint-dashboard at runtime if env var is present - Plugins can now add config to the console-info call - Add backend component to endpoint-dashboard -- If env var 'DISABLE_ENDPOINT_DASHBOARD' is present add 'disabled' to component config - Handle disabled dashboard in frontend.. -- Do not add to menu -- Do not redirect to dashboard if no endpoints present -- Remove from env.plugins - Fixed endpoint on logged in message for admin's when there are no connected endpoints - Speed up logged in process by making 3 calls async - Improved unit tests
  Testing concourse
  Added ulimit note to development readme
KlapTrap added a commit that referenced this pull request Aug 22, 2017
@KlapTrap
Merge branch 'master' into sass-source-maps
ff88520 
* master:
  Reuse unbound routes (#1223)
  Revert "Added ulimit note to development readme"
  Show services in app summary page, auto pick service plan in add service modal
  Changes following review
  If the value of a 'select-input' is a placeholder always translate
  Fix e2e
  Moved logic into specific cloud-foundry-hosting plugin
  Final bits n bobs - Switch logic from 'disable' flag to 'enable' (true by default) - Updated docs - Prooved cf push with/wuthout flag
  Fix e2e
  Disable the endpoint-dashboard at runtime if env var is present - Plugins can now add config to the console-info call - Add backend component to endpoint-dashboard -- If env var 'DISABLE_ENDPOINT_DASHBOARD' is present add 'disabled' to component config - Handle disabled dashboard in frontend.. -- Do not add to menu -- Do not redirect to dashboard if no endpoints present -- Remove from env.plugins - Fixed endpoint on logged in message for admin's when there are no connected endpoints - Speed up logged in process by making 3 calls async - Improved unit tests
  Testing concourse
  Added ulimit note to development readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-cox richard-cox richard-cox approved these changes

Assignees
No one assigned
Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KlapTrap @richard-cox