Sachin/automate infra proxy server ui #2581

sachin-msys · 2019-12-26T13:13:52Z

🔩 Description: What code changed, and why?

Code changes for introduction of infra proxy module in automate-ui In order to make a single source of truth for chef infra servers data here

In this PR following points we have covered:

Created infra-proxy module in automate-ui to keep all infra-proxy related component separate
Designed infra proxy UI flows and used Infra-proxy-service

⛓️ Related Resources

Refered Wireframes: https://chef.invisionapp.com/share/PZSLN5TJ8B9#/screens/369724917
For feature, description refer: #1544

[ Updated Mock Ups Jan 17, 2020 ] - addition of Cookbooks to the InVision prototype
https://chef.invisionapp.com/share/3JVLVFMASDZ#/401258846_V0_-_Client_Runs_-_Cookbooks_-_Details_-_Description
Note: The side nav may different because ideally would love to aggregate the cookbooks from all orgs.

👍 Definition of Done

I have added infra proxy module and UI new pages

chef server's list page
create chef-server popup modal

👟 How to Build

build components/infra-proxy-service
rebuild components/automate-gateway
rebuild components/automate-deployment
build components/automate-ui

NOTE: This PR is dependent on PR #2173 so build steps are depending on the same for Infra-proxy-service

msorens

Looking good; some comments noted below.

components/automate-ui/src/app/entities/servers/server.effects.ts

components/automate-ui/src/app/entities/servers/server.reducer.ts

...nts/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.ts

msorens · 2020-01-03T17:30:35Z

components/automate-ui/src/app/helpers/auth/regex.ts

+    // Allows valid FQDN only
+    VALID_FQDN: '(https?://)?([\\da-z.-]+)\\.([a-z.]{2,6})[/\\w .-]*/?',
+
+    // Allows valid IP Address only (ipv4)
+    VALID_IP_ADDRESS: '^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$'


If you copied these from a published source it is important to add a comment noting the source.
If you created these from scratch, I think it would be really good to add tests (at least a couple dozen for each) that show what does and does not validate. That will let folks who are less familiar with reading regex's be able to judge whether it meets product needs.

Just a few problem examples off-hand for FQDN:

These should match but do not:

http://fo_o.bar/ https://bit.ly/2OWGwiL

These should not match but do:

http://foo.bar com/ http://foo.bar..com/ http://...foo.bar.com/ http://foo.bar-.-.

Probably better to find a reliable, documented source rather than create your own to avoid such problem areas. 😁

Hello @msorens
I have rewrote the REGX for FQDN but still not able to create a perfect one so can I get a help on the same please.

I did a bit of web searching and found a couple possibilities but because of the nature of an FQDN there does not seem to be a consensus of a good matching regex. So my suggestion is let's back up; you should chat with whoever is requesting this work (is that @jonong1972 ?) to find out what types of FQDN's are needed. It may be that for chef servers we can more precisely specify a URL.

I found that we use this in a couple spots in the code base:

automate/components/automate-ui/src/app/pages/notification-form/notification-form.component.html

Line 97 in f8ca770

pattern="https?://[^.]+\..+"

automate/components/automate-ui/src/app/pages/data-feed-form/data-feed-form.component.html

Line 37 in f8ca770

pattern="https?://[^.]+\..+"

which, in English means: either http:// or https:// followed by something-dot-something.
It is a very forgiving pattern--it will not exclude any valid ones, which is important--though it will certainly allow invalid ones.

@jonong1972 Do you think that pattern would be sufficient here?

@msorens created followed regx which is allowing most of valid ones and blocking most of invalid one

VALID_FQDN: /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,5}(:[0-9]{1,5})?(\/.*)?$/,

tried and added test cases for the examples which you have provided
Valid Inputs

http://fo_o.bar/ https://bit.ly/2OWGwiL

Invalid Inputs

http://foo.bar com/ http://foo.bar..com/ http://...foo.bar.com/ http://foo.bar-.-.

working well with above all can we go with this ?

I want to get some product or UX guidance on this question--I will try to speak to @jonong1972 about this tomorrow.

@sachin-msys lets not do any validation on this input right now, we can come back and iterate later after this work is out of discovery.

@susanev yes sure sounds good, for now resolved conflict please have a look thanks

msorens · 2020-01-03T17:34:11Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

+
+  it('should create', () => {
+    expect(component).toBeTruthy();
+  });


Please add some tests. The functionality of the underlying component is simple, yes, but should still be tested. You should be able to adapt existing tests for other components that do very similar things.

Nice tests; thank you!

jonong1972 · 2020-01-17T21:48:56Z

I updated the description with a link to the Cookbooks section. We can discuss in next meeting of the small things.

vjeffrey · 2020-01-29T17:42:01Z

yesterday we discussed trying to get this pr merged asap to get some bits merged and try to move towards a more iterative approach.
it looks like there's a conflicting file blocking this.
if there are more unanswered questions/feedback, we can create issues to address those next to support moving forward.
please let me know if you need anything from me to get that done!

Signed-off-by: Sachin Bachhav <[email protected]>

SEAjamieD

Still going through but pushing this through now so you can get started - have a few changes that will be needed because of more recent UI changes specifically related to chef-control-menu.

SEAjamieD · 2020-01-29T21:55:45Z

...s/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.html

+                <chef-control-menu>
+                  <chef-option data-cy="delete" (click)="startServerDelete(server)">Remove Chef Server</chef-option>
+                </chef-control-menu>


We are no longer using chef-control-menu. It has been replaced with Angular Material.

suggest using:

<mat-select panelClass="chef-control-menu"> <mat-option data-cy="delete" (onSelectionChange)="startServerDelete($event, server)">Remove Chef Server</mat-option> </mat-select>

You'll also need to pass in the event, so that in chef-servers-list.component.ts you can add a gate for isUserInput

you can find an example of that here: https://github.com/chef/automate/pull/2283/files in project-list.component.ts.

Happy to explain more if necessary. Here is an example of it in use: #2283 (comment)

SEAjamieD · 2020-01-29T21:57:12Z

...s/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.scss

+chef-table-new {
+  .three-dot-column {
+    text-align: right;
+  }
+}


This is now global, so it can be deleted from this file all together 😄

SEAjamieD · 2020-01-29T22:02:03Z

...nts/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.ts

+  public startServerDelete(server: Server): void {
+    this.serverToDelete = server;
+    this.deleteModalVisible = true;
+  }


related to my comment above, since we are now using mat-select instead of chef-control-menu, we need to put a guard in place or we'll get two event fires. Suggest changing this function to look like this:

public startServerDelete($event: ChefKeyboardEvent, server: Server): void { if ($event.isUserInput) { this.serverToDelete = server; this.deleteModalVisible = true; } }

The reason for this is that when using mat-select, if fires off two events, one for a selection and one for a deselection. $event.isUserInput = true is the user selected event.

@SEAjamieD Thanks for the review. I have incorporated all suggested changes please have a look

Thanks for the changes - they look great 👍

Signed-off-by: Sachin Bachhav <[email protected]>

SEAjamieD

The changes I requested look good 🎉

msorens

Looking good!
The one thing I noticed is that there was a decision to remove validation on URLs but that validation is still present. I am approving anyway; leave the call on that change for @susanev since she had the last word there.

For future work: it would be really, really helpful if you provide instructions on how to exercise this code. Not just what to rebuild, but precise steps on what to do after logging in on the browser. I would have liked to run this code but was unable to, not being familiar with it.

msorens · 2020-01-30T21:25:22Z

...utomate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

+
+  it('should create', () => {
+    expect(component).toBeTruthy();
+  });


Nice tests; thank you!

msorens · 2020-01-30T21:33:00Z

...i/src/app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.ts

+  templateUrl: './create-chef-server-modal.component.html',
+  styleUrls: ['./create-chef-server-modal.component.scss']
+})
+export class CreateChefServerModalComponent implements OnInit {


Comment for future work--not for now:

This code follows our existing patterns fine. But we recently introduced a new, cleaner pattern that I would like you and your team to be aware of for modals--see #2704. It encapsulates the code for the modal in a much cleaner way.

Again, no need to apply to this PR; sounds like we want to get this merged ASAP. But please take a look in case of future modals.

sachin-msys changed the title ~~added chef infra server module in automate~~ Scahin/automate infra proxy server ui Dec 26, 2019

sachin-msys force-pushed the Sachin/automate-infra-proxy-server-ui branch from e44e766 to f37785b Compare December 27, 2019 09:39

sachin-msys self-assigned this Dec 27, 2019

sachin-msys added automate-infra-proxy automate-ui ui labels Dec 27, 2019

vinay033 mentioned this pull request Jan 3, 2020

Vinay/automate infra proxy org ui #2605

Closed

msorens reviewed Jan 3, 2020

View reviewed changes

sachin-msys force-pushed the Sachin/automate-infra-proxy-server-ui branch from 610144c to ac8f117 Compare January 14, 2020 21:21

sachin-msys changed the title ~~Scahin/automate infra proxy server ui~~ Sachin/automate infra proxy server ui Jan 14, 2020

sachin-msys force-pushed the Sachin/automate-infra-proxy-server-ui branch 2 times, most recently from 1263cf3 to 1a265a5 Compare January 23, 2020 11:22

sachin-msys added 5 commits January 30, 2020 00:19

added chef infra server module in automate

519efbb

Signed-off-by: Sachin Bachhav <[email protected]>

added FQDN and IP Address validations

af91f53

Signed-off-by: Sachin Bachhav <[email protected]>

env const added for infra proxy url

4f23b3d

Signed-off-by: Sachin Bachhav <[email protected]>

Added tests for component and FQDN field

dac3d1a

Signed-off-by: Sachin Bachhav <[email protected]>

added some more test cases to validated FQDN field inputs

202f229

Signed-off-by: Sachin Bachhav <[email protected]>

sachin-msys force-pushed the Sachin/automate-infra-proxy-server-ui branch from 1a265a5 to 202f229 Compare January 29, 2020 18:54

SEAjamieD suggested changes Jan 29, 2020

View reviewed changes

replced chef-control-menu with angular mat-select

dc7ec4e

Signed-off-by: Sachin Bachhav <[email protected]>

susanev requested review from SEAjamieD and msorens January 30, 2020 19:15

SEAjamieD approved these changes Jan 30, 2020

View reviewed changes

susanev requested review from jonong1972 and removed request for jonong1972 January 30, 2020 19:19

susanev approved these changes Jan 30, 2020

View reviewed changes

msorens approved these changes Jan 30, 2020

View reviewed changes

susanev merged commit f414394 into master Jan 30, 2020

chef-expeditor bot deleted the Sachin/automate-infra-proxy-server-ui branch January 30, 2020 21:45

btm mentioned this pull request Mar 4, 2020

Scahin/automate infra proxy ui #2209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sachin/automate infra proxy server ui #2581

Sachin/automate infra proxy server ui #2581

sachin-msys commented Dec 26, 2019 •

edited by jonong1972

Loading

msorens left a comment

msorens Jan 3, 2020

sachin-msys Jan 14, 2020

msorens Jan 21, 2020

msorens Jan 21, 2020

sachin-msys Jan 28, 2020

msorens Jan 29, 2020

susanev Jan 29, 2020

sachin-msys Jan 29, 2020 •

edited

Loading

msorens Jan 3, 2020

msorens Jan 30, 2020

jonong1972 commented Jan 17, 2020

vjeffrey commented Jan 29, 2020

SEAjamieD left a comment

SEAjamieD Jan 29, 2020

SEAjamieD Jan 29, 2020

SEAjamieD Jan 29, 2020

sachin-msys Jan 30, 2020

SEAjamieD Jan 30, 2020

SEAjamieD left a comment

msorens left a comment

msorens Jan 30, 2020

msorens Jan 30, 2020

Sachin/automate infra proxy server ui #2581

Sachin/automate infra proxy server ui #2581

Conversation

sachin-msys commented Dec 26, 2019 • edited by jonong1972 Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachin-msys Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonong1972 commented Jan 17, 2020

vjeffrey commented Jan 29, 2020

SEAjamieD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD left a comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachin-msys commented Dec 26, 2019 •

edited by jonong1972

Loading

sachin-msys Jan 29, 2020 •

edited

Loading