Skip to content

Conversation

@pcbailey
Copy link
Contributor

@pcbailey pcbailey commented Nov 1, 2019

This PR displays an informational message when no network types are available which directs the user to use the YAML editor instead.

Depends on #3183

OKD - Google Chrome_624

@phoracek @matthewcarleton

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2019
@pcbailey pcbailey force-pushed the add-message-when-no-network-types-available branch from 576cb2b to 6f69948 Compare November 1, 2019 15:29
@lizsurette
Copy link

@pcbailey Would it make sense to load the YAML Editor and include the notification at the top to example why the YAML Editor is showing rather than the form? One less click if we could pull it off :)

Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

I agree with Liz, but this state is fine as well :)

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, phoracek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pcbailey
Copy link
Contributor Author

pcbailey commented Nov 3, 2019

@lizsurette @matthewcarleton @phoracek
Does this work for you guys? Ignore the copy. I wasn't sure what it should say. Input there would be very helpful and appreciated.

OKD - Google Chrome_625

@phoracek
Copy link
Member

phoracek commented Nov 3, 2019

Looks nice to have it on the same page, just a couple of notes.

The yaml should show only the empty "config"': '{}', so users have to provide a "type" they know is available on the cluster.

I'm not sure whether we should mention HyperConverged at all there, it is not supported component of OpenShift (unlike SR-IOV). If we mentioned only SR-IOV we'd be on the safe side. However, I suppose this text would not harm either (more of a doc/ux team question).

@pcbailey
Copy link
Contributor Author

pcbailey commented Nov 3, 2019

@phoracek The branch this screenshot was taken from isn't rebased on top of the one used for #3193 that updates the YAML template, which is why it's still showing all the config details.

I figured there would probably be concern around naming the specific operators, but that makes it tricky because one of them is required to activate the form. So, I guess as long as we can mention at least one of them, it shouldn't be that difficult. @matthewcarleton @lizsurette Any thoughts on what the message should say?

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 4, 2019

@pcbailey So the user arrives here via the "Create Network Attachment Definition" action. It is not a dropdown that gives them the option of form VS YAML right?
Maybe adjust the text to say:
Form not available
In order to create a Network Attachment Definition via a form please Install either the Hyper Converged Cluster or SR-IOV Operator.
This focuses more on Why the alert is there for users who expected the form but didn't get it, Do you think we need to include information about Network types?
Can we link off to the operators to make it a bit easier for them to accomplish the task?
Should this alert be dismissible?
Nitpic: there looks to be some spacing issues too where the borders aren't flush with the edge of the content area.
Also for the design I think the alert should go below the Page title section, I'm not sure if there's any docs around this for the console but it feels a bit odd to me. Here are some other options:
Option 1
Screen Shot 2019-11-04 at 10 13 21 AM
Option 2
Screen Shot 2019-11-04 at 10 13 35 AM

@lizsurette WDYT?

@lizsurette
Copy link

@lizsurette WDYT?

Are you design options different @matthewcarleton ? I'm trying to look closely (squinting) and not seeing the difference haha.

I totally agree with the placement of the notification being closer to the YAML form to make it clear what it's applying to!

I also really like the text suggestions you have made. So +1 to me on everything @matthewcarleton wrote :)

@matthewcarleton
Copy link
Contributor

@lizsurette I've updated the second image 🤦‍♂

@lizsurette
Copy link

@lizsurette I've updated the second image 🤦‍♂

Nice, thanks! I'm leaning towards Option 2 since it applies to the Form and Preview area...are you leaning either way?

Any preference on your side @pcbailey ?

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 5, 2019

@lizsurette ya 2 seems like the right choice. It would be worth it to dig around and see if there are any current examples of notifications on YAML pages and what they look like too. I can do that and report back.

@pcbailey
Copy link
Contributor Author

pcbailey commented Nov 5, 2019

@matthewcarleton @lizsurette

Matt, you're correct. The user gets here through the "Create Network Attachment Definition" action and not a dropdown.

I'm good with the wording and I'll see about the possibility of linking to the SR-IOV operator. @phoracek We're avoiding mentioning the HCO operator, right?

I agree that the alert looks better under the header. The problem is that doing it that way would require me to modify the core console YAML component, which may tie the PR up longer due to requiring a review by the console team. I'll look again and see if there's any easier way to do it, though. I'll also look into the spacing issues.

@phoracek
Copy link
Member

phoracek commented Nov 5, 2019

Correct. It feels weird to tell users "Install whole HCO to be able to use this form for Linux bridge". It should come only with HCO if users want HCO. Hopefully, the bridge plugin will become a part of OpenShift one day.

@pcbailey
Copy link
Contributor Author

pcbailey commented Nov 5, 2019

@phoracek I agree that it's weird. Are there any other types that are supported out of the box by OpenShift that we could add to avoid this issue altogether?

@pcbailey pcbailey changed the title Display message when no network types available WIP: Display message when no network types available Nov 5, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2019
@phoracek
Copy link
Member

phoracek commented Nov 5, 2019

I believe they are, but we'd need someone from OpenShift networking to share more details.

@squeed OpenShift Console now has dialog for listing and creating of NetworkAttachmentDefinitions. There is a custom dialog for SR-IOV and for CNV's linux bridge (when respective operators are installed). Is there a CNI plugin that is installed by default with Multus? Maybe macvlan? We are looking for one CNI plugin installed by default, so we always have something to show in "Network type" dropdown.

@openshift-ci-robot
Copy link
Contributor

@pcbailey: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/frontend 6f69948 link /test frontend
ci/prow/e2e-gcp-console 6f69948 link /test e2e-gcp-console
ci/prow/analyze 6f69948 link /test analyze

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@pcbailey pcbailey changed the title WIP: Display message when no network types available Bug 1770919: WIP: Display message when no network types available Nov 11, 2019
@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 11, 2019
@openshift-ci-robot
Copy link
Contributor

@pcbailey: This pull request references Bugzilla bug 1770919, which is invalid:

  • expected the bug to target the "4.3.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1770919: WIP: Display message when no network types available

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@squeed
Copy link

squeed commented Nov 11, 2019

Woah, this is a bit of a surprise. Do we have a user story for this? What is the expected functionality?

NetworkAttachDefinitions are low-objects that administrators can use to create additional network interfaces inside pods. However, the network plugins need to be installed by higher-level components. These components, such as HCV , SR-IOV, and others, create NetworkAttachmentDefinitions as part of their networking configuration.

However, there's a lot that can go wrong when creating them. You need to understand your addressing requirements. Plugins are cloud-provider and node-hardware specific. We prefer for end-users to create higher-level objects that are interpreted by an operator.

That's what we do for SR-IOV, macvlan, and Istio: we manage them with operators. I can't think of a safe way to build them in a simple web form.

@fepan

@phoracek
Copy link
Member

@squeed we in CNV/KubeVirt want an UI for bridge CNI to provide user simple way to create NetworkAttachmentDefinitions with sane defaults. Looking forward, we want to allow users to connect their VMs to SR-IOV too, therefore we added it to the UI too. We considered using SriovNetwork, but it would require us to create a separate section for each network type.

@pcbailey
Copy link
Contributor Author

The network attachment definitions UI will only be displayed when CNV is installed, which will ensure that HCO support is available. Closing this PR as the changes are no longer necessary.

@pcbailey pcbailey closed this Nov 13, 2019
@pcbailey pcbailey deleted the add-message-when-no-network-types-available branch November 13, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants