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

Add Application Autoscaler Tab #3455

Merged
merged 116 commits into from
Jul 24, 2019
Merged

Add Application Autoscaler Tab #3455

merged 116 commits into from
Jul 24, 2019

Conversation

richard-cox
Copy link
Contributor

See #3266 for additional history

@cfdreddbot
Copy link

✅ Hey richard-cox! The commit authors and yourself have already signed the CLA.

Copy link
Contributor Author

@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.

Not quite made it through yet, but here's a few very minor non-functional suggestions

Copy link
Contributor Author

@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.

Along with inline comments there's some general things, all are minor and I don't think need to be done for the demo.

Autoscaler Tab

  • Refresh on slower loading connections and the policy slider in status is shown. This should be hidden as the rest See LoadingPageComponent appears underneath content with z-index > 0 #3509
  • The status card shows a 'policy' slider with text 'Policy undefined' or 'Policy defined'. Think this text should just be 'Enabled' and 'Disabled' for the two states 78405f3
  • Latest Events Card
    • When there are no events we should probably not show the link in the bottom right 78405f3
    • The position of the link button appears over the table when there's no time based rules 78405f3
  • Scheduled Limit Rules Card
    • The position of the edit button appears over the table when there's no time based rules 78405f3

AutoScaler Metric Charts Modal

  • On first load the chart flashes three or four times. I think this could be a recent change we made that means observable chains on entities fire more often. I'm looking into this

Edit AutoScaler Policy Stepper

  • Scaling Rules, Recurring Schedules and Specific Dates steps
    • I think we need to bring the 'Add' button up from the stepper controls to the same context as the list of rules. Something like an 'add' material icon with the text 'Add' in a button bottom left of the list would look good. d0ca5f6
  • Specific Dates Step
    • On clicking finish
      • Success - User should be taken back to the Autoscaler tab. Think you can do this by setting redirect: true in the StepOnNextResult returned by the step onNext function d141f1d
      • Failure - the onNext function should return { success: false, message: ''}. That will show the error in a snackbar at the bottom of the screen instead of in the main context d141f1d

src/tsconfig.json Outdated Show resolved Hide resolved
@zyjiaobj zyjiaobj force-pushed the autoscaler branch 3 times, most recently from 3bcbeb2 to f18d245 Compare March 31, 2019 13:44
@zyjiaobj zyjiaobj force-pushed the autoscaler branch 8 times, most recently from e6813dc to d0ca5f6 Compare April 11, 2019 07:03
@KlapTrap KlapTrap self-requested a review June 11, 2019 08:57
Copy link
Contributor

@KlapTrap KlapTrap left a comment

Choose a reason for hiding this comment

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

I'm a little nervous about the small amount of FE & BE unit tests. Unit tests are even more important when we don't own the code, it would really help us out when maintaining our codebase in the future.

loadChildren: './core/autoscaler.module#AutoscalerModule',
data: {
stratosNavigation: {
text: 'Applications',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add an entry to the stratosNavigation here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean to remove the below code?

    data: {
      stratosNavigation: {
        text: 'Applications',
        matIcon: 'apps',
        position: 20,
        hidden: of(true)
      }
    },

But when i remove this part of code, the https://localhost:4200/autoscaler/b5F2dF-Qr-r7yNWeDya0GTOL09c/2369e308-0733-40ce-ae9e-ed85fdbcca40/edit-autoscaler-policy and other autoscaler paths will failed to load with 404 error.
I don't know why... Could you please help me check? @richard-cox @KlapTrap

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I thought. I think it would be worth rich and I taking a look and seeing what's going on, it doesn't seem right for you to have to do that!

src/frontend/packages/core/src/core/utils.service.ts Outdated Show resolved Hide resolved
@KlapTrap KlapTrap added needs attention This PR needs attention and removed ready for review labels Jun 12, 2019
@zyjiaobj zyjiaobj force-pushed the autoscaler branch 2 times, most recently from deddf28 to 89a3000 Compare June 21, 2019 03:17
@richard-cox
Copy link
Contributor Author

I've resolved the merge conflicts by bringing in v2-master. This has brought with it a change that breaks connecting to IBM Cloud via the endpoints page (SSO/cf push works fine though). There's a PR up that will fix this - #3715. I'll work on getting this through the gates again

@richard-cox richard-cox added ready for review and removed needs attention This PR needs attention labels Jul 16, 2019
@zyjiaobj zyjiaobj force-pushed the autoscaler branch 2 times, most recently from b7edbff to c05f44f Compare July 22, 2019 08:35
- Ensure autoscaller tiles take up full width when page is shrunk horizontally
- position of polling indicator on recent apps
- app-tile margin removed from top and added to bottom (fixes app/cf summary pages)
- added min height to latest metrics card to avoid vertical wibble on refresh
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants