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.

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

Tp interface servers #4747

Merged
merged 34 commits into from
Jun 30, 2020
Merged

Conversation

ocket8888
Copy link
Contributor

@ocket8888 ocket8888 commented Jun 1, 2020

What does this PR (Pull Request) do?

Updates Traffic Portal tables and forms to account for servers having multiple interfaces, including:

  • any table that shows servers obtained from /servers such as
    • Servers
    • Cache Groups -> View Servers
    • Profiles -> View Servers
    • Delivery Services -> View Servers
  • The server create/edit form

Which Traffic Control components are affected by this PR?

  • Traffic Portal

What is the best way to verify this PR?

  • Build Traffic Portal, verify that all the mentioned tables and forms work properly.
  • Run the TP integration tests.
  • You could also run the UI tests, but I think they're broken on master atm [citation needed], so I'll need to look into that.

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch 2 times, most recently from aaf175c to 39401fe Compare June 9, 2020 18:39
@ocket8888 ocket8888 added improvement The functionality exists but it could be improved in some way. medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Portal v1 related to Traffic Portal version 1 labels Jun 9, 2020
@ocket8888 ocket8888 marked this pull request as ready for review June 9, 2020 19:08
@ocket8888 ocket8888 mentioned this pull request Jun 10, 2020
7 tasks
@mitchell852
Copy link
Member

@ocket8888 - do you want to move the The ISO generation form part into it's own PR to keep this PR focused?

@ocket8888
Copy link
Contributor Author

I suppose I can. Working on something else atm, though, so it might be a bit.

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch from 39401fe to bef4eb3 Compare June 10, 2020 23:20
@ocket8888
Copy link
Contributor Author

Done

Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

A few things:

  1. offline reason label looks screwed up:

image

  1. how about putting some words on these buttons rather than + and - so you don't have to hover over the button to figure out what it's for:

image

  1. how about moving the interfaces section to the bottom of the form rather than putting it between profile and phys location?

Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

getting errors on the server create page - https://tp.domain.com/#!/servers/new

first error:

TypeError: Cannot read property 'cdnId' of undefined
at init (FormServerController.js:205)

Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

some more feedback

<div class="modal-footer">

<fieldset>
<legend>Interfaces</legend>
Copy link
Member

@mitchell852 mitchell852 Jun 15, 2020

Choose a reason for hiding this comment

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

when you add your first interface, it's unclear what to put in the first box

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should I do about that? Add a placeholder or a label? I thought using the name as the full legend was pretty neat, looked a little weird with "Name: " in front of everything.

Copy link
Member

@mitchell852 mitchell852 Jun 22, 2020

Choose a reason for hiding this comment

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

placeholder would be fine. i just think it will confuse people. maybe placeholder: name (i.e. eth0, bond0, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeholders are only supposed to be example values that would be valid in the input if you typed them in. So I added a placeholder, lemme know if you think that works.

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch from 1f321a4 to d285f33 Compare June 15, 2020 21:49
@ocket8888
Copy link
Contributor Author

I'm not seeing any errors on the server create page. Tried Chrome and Firefox.

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch 2 times, most recently from 0dc19f9 to c099126 Compare June 17, 2020 21:18
@mitchell852
Copy link
Member

mitchell852 commented Jun 22, 2020

I like the way you've nested the interfaces on the form, but i'm still have a hard time visually lining up the buttons with the proper nested box and the interface buttons are interfering with the main buttons of the form as well (update/delete).

image

what about moving a button or a link into the legend like such:

image

@mitchell852
Copy link
Member

also, i'm getting this error when trying to create a server:

shared-libs.js:65171 TypeError: Cannot read property 'cdnId' of undefined

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch from c099126 to f242110 Compare June 22, 2020 19:56
<legend>Interfaces<button class="btn btn-success right-button" type="button" title="add a new interface" ng-click="addInterface()">Add Interface</button></legend>
<fieldset ng-repeat="inf in server.interfaces">
<legend>
<input my-directive type="text" ng-model="inf.name" required aria-label="Interface name" id="{{inf.name}}-name" name="{{inf.name}}-name" ng-class="{'has-error': hasError(serverForm[inf.name+'-name'])}" placeholder="eth0"/>
Copy link
Member

Choose a reason for hiding this comment

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

my-directive?

Copy link
Member

Choose a reason for hiding this comment

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

this text input acts weird for me. does it for you? i can only put my cursor at the start of the text.

Copy link
Contributor Author

@ocket8888 ocket8888 Jun 22, 2020

Choose a reason for hiding this comment

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

idk what "my-directive" is or why it's in there. I'll get rid of it.
as far as "acting weird," no, I don't think I have noticed that, but I'll look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

directive is gone, can't reproduce not being allowed to put your cursor wherever you want

Copy link
Member

Choose a reason for hiding this comment

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

oh, i understand what is going on

image

eth0 is a placeholder. it looks like regular text so i was trying to double click it to select it all to replace it...

Copy link
Member

Choose a reason for hiding this comment

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

maybe the placeholder was a bad idea because now it looks like text and the update or create button is disabled unless i change it....but if i want it to be eth0 it looks like it's already filled in. maybe a better placeholder would be interface-name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work.

@ocket8888
Copy link
Contributor Author

also, i'm getting this error when trying to create a server:

shared-libs.js:65171 TypeError: Cannot read property 'cdnId' of undefined

fixed

@mitchell852
Copy link
Member

mitchell852 commented Jun 23, 2020

If you put your buttons in the legend, i think the icon buttons (like you had originally) work pretty well instead of the words. looks cleaner

image

for example:

<legend>Interfaces<a title="add a new interface" class="btn btn-primary btn-xs pull-right" ng-click="addInterface()"><i class="fa fa-plus"></i></a></legend>

@mitchell852
Copy link
Member

I created a PR for you if you want to show the success/error message in a more friendly way

ocket8888#36

@ocket8888
Copy link
Contributor Author

Buttons are back to +/-

@mitchell852
Copy link
Member

@ocket8888 - doesn't every cache require at least one service address? I can create one w/o a service address.

image

@mitchell852
Copy link
Member

I added the 42 to an existing interface and the Update button did not enable:

image

@ocket8888 ocket8888 force-pushed the tp-interface-servers branch from b44054d to 801417e Compare June 25, 2020 22:32
@mitchell852
Copy link
Member

ocket8888#38

@mitchell852
Copy link
Member

mitchell852 commented Jun 26, 2020

also, getting this error when trying to update status on a server:

image

image

@ocket8888 did you look at this one?

actually, just apply this @ocket8888 - mitchell852@e0f33ad - i tried to PR your PR but it got all screwed up.

@mitchell852
Copy link
Member

"I can create one w/o a service address."

No you can't. You can try, but it's not possible.

No, I did. It was possible. :)

ok, this is fixed now with #4828

Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

UI tests pass. did a bunch of manual testing. everything seems to work as expected. nice work!

@mitchell852
Copy link
Member

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement The functionality exists but it could be improved in some way. medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Portal v1 related to Traffic Portal version 1
Projects
None yet
2 participants