[BUG] settings need reload to actually happen

Describe the bug
A clear and concise description of what the bug is.
before, when toggling the booleans that I  have configured to control the Kiosk mode options, the toggle would immediately change the frontend.
As of late, I need to reload the view for the change to appear.
Also, when not reloading but changing another option, that causes the former toggle to kick in. So it seems as if the wrong boolean was toggled

Console log errors (F12)
No errors

To Reproduce
Steps to reproduce the behavior:

Go to '...'
Click on '....'
Scroll down to '....'
See error

Expected behavior
A clear and concise description of what you expected to happen.
toggle immediately changing frontend. As it has always done
Screenshots
If applicable, add screenshots to help explain your problem.

Versions:

Home Assistant: 2024.9.1

Kiosk-Mode 6.1.1

Additional context
Add any other context about the problem here.
my kiosk mode settings (which I havent changed since march 2023).... :
# https://github.com/maykar/kiosk-mode#config-options
# https://github.com/NemesisRE/kiosk-mode
# admin_settings:
# #  ignore_mobile_settings: true
#   hide_sidebar: false

# hide_assistant: true

non_admin_settings:
  hide_sidebar: true
  hide_menubutton: true
  hide_overflow: true
  hide_assistant: true
  hide_search: true
#   ignore_entity_settings: true

mobile_settings:
  hide_menubutton: true
  hide_overflow: true
#   ignore_entity_settings: true

entity_settings:

  - entity:
      binary_sensor.unhide_sidebar_for_non_admin: 'on'
    hide_sidebar: false

#Hides both the header and sidebar.
  - entity:
      binary_sensor.kiosk_mode_selected: 'on'
#      input_select.mode: Kiosk
    kiosk: true
# Hides only the header
  - entity:
      input_boolean.hide_header: 'on'
    hide_header: true
# Hides only the sidebar. Disables swipe to open.
  - entity:
      input_boolean.hide_sidebar: 'on'
    hide_sidebar: true
# Hides only the sidebar menu icon. Does not disable swipe to open.
  - entity:
      input_boolean.hide_menubutton: 'on'
    hide_menubutton: true
# Hides the top right menu
  - entity:
      input_boolean.hide_overflow: 'on'
    hide_overflow: true
# Hides the account icon
  - entity:
      input_boolean.hide_account: 'on'
    hide_account: true
# Hides notification tray
  - entity:
      input_boolean.hide_notitications: 'on'
    hide_notifications: true
# Hides the search icon
  - entity:
      input_boolean.hide_search: 'on'
    hide_search: true
# Hides the assistant icon
  - entity:
      input_boolean.hide_assistant: 'on'
    hide_assistant: true
# Hides refresh menu item
  - entity:
      input_boolean.hide_refresh: 'on'
    hide_refresh: true
# Hides the unused entities menu item
  - entity:
      input_boolean.hide_unused_entities: 'on'
    hide_unused_entities: true
# Hides the reload resources menu item
  - entity:
      input_boolean.hide_reload_resources: 'on'
    hide_reload_resources: true
# Hides edit dashboard menu item
  - entity:
      input_boolean.hide_edit_dashboard: 'on'
    hide_edit_dashboard: true
# Block mouse on empty overflow menu-items
  - entity:
      binary_sensor.kiosk_mode_block_overflow: 'on'
    block_overflow: true
# Block mouse on overflow menu-items
  - entity:
      input_boolean.kiosk_mode_block_mouse: 'on'
    block_mouse: true

  - entity:
      input_boolean.hide_dialog_header_action_items: 'on'
    hide_dialog_header_action_items: true
  - entity:
      input_boolean.hide_dialog_header_history: 'on'
    hide_dialog_header_history: true
  - entity:
      input_boolean.hide_dialog_header_settings: 'on'
    hide_dialog_header_settings: true
  - entity:
      input_boolean.hide_dialog_header_overflow: 'on'
    hide_dialog_header_overflow: true

  - entity:
      input_boolean.hide_dialog_history: 'on'
    hide_dialog_history: true
  - entity:
      input_boolean.hide_dialog_history_show_more: 'on'
    hide_dialog_history_show_more: true

  - entity:
      input_boolean.hide_dialog_logbook: 'on'
    hide_dialog_logbook: true
  - entity:
      input_boolean.hide_dialog_logbook_show_more: 'on'
    hide_dialog_logbook_show_more: true

  - entity:
      input_boolean.hide_dialog_attributes: 'on'
    hide_dialog_attributes: true

  - entity:
      input_boolean.hide_dialog_media_actions: 'on'
    hide_dialog_media_actions: true
  - entity:
      input_boolean.hide_dialog_update_actions: 'on'
    hide_dialog_update_actions: true
  - entity:
      input_boolean.hide_dialog_climate_actions: 'on'
    hide_dialog_climate_actions: true
  - entity:
      input_boolean.hide_dialog_timer_actions: 'on'
    hide_dialog_timer_actions: true

Hi @Mariusthvdb,
You need to check what is happening on your side and what provoked that, but the entity_settings are working perfect on my Home Assistant instance without any reload (2024.9.1). But just to be double sure I opened the demo that we have set up here in the repo for the end-to-end tests which is now running on 2024.9.1 too. As this is a fresh instance just with the plugin installed is the best one to perform these tests:

Take into account that the last release was in Agugust 20th (which contained only dependencies updates to dev packages not related to the plugin code), the real change to the code was in v6.1.0 on July 22th. When did you notice that behaviour in your instance?
Regards

ill try to drop a screen vid, so you can see what's happening.
tbh, most of my frontend stuff is automated, so I hardly ever manually toggle them. but I noticed today, because someone else in the community said Kiosk mode didnt work

Well, the library is not the culprit for what is ocurring to you, I can asure you that. The end-to-end tests that we have in place would break if something like that occurs and they are working even with the Nightly Beta Tests. Let's try to figure it out what is provoking that behaviour on your side. I'll record the screen so you can do the same on your side and learn how to debug that.
https://github.com/user-attachments/assets/3bfe26f7-9fc0-4119-a684-3755abe8d9e0
Verify that the proper style element is being added to that shadow root.

wild guess: could it have to do with Sections?
recently I changed from default (masonry) or custom layout views to sections on many views, also this particular view:

Just check your layout to see if it looks like the one in the video, we are going to figure it out, no worries.

quick feedback:
it's not my theming: ive tested using default theme (homeassistant default) and behavior is identical
it's not the layout: returning to custom layout doesnt change behavior
any toggle is 1 state change behind
need to check your vid, will do next

well, this is unexpected, Ive found the issue:
I've added a custom resource of myself recently, that can inject templates on attributes, which I need for 1 specific reason.
Disabling that immediately returns the correct functionality to Kiosk-mode.....
Which is great on the 1 hand, since this plugin is not flawed 🎉
Otoh...
@elchininet could I ask you to please have a look at the new feature  and the resource js file ?
it is a miniature of the former custom-ui that never caused anything like this, so maybe I made an error after all there.
Closing this issue here

I can confirm that
kiosk_mode:
  kiosk: true

works but following doesn't
kiosk_mode:
  entity_settings:
    - entity: input_select.theme_selector
      state: ios-dark-mod
      hide_sidebar: true
      hide_header: true

well, this is unexpected, Ive found the issue:
I've added a custom resource of myself recently, that can inject templates on attributes, which I need for 1 specific reason.
Disabling that immediately returns the correct functionality to Kiosk-mode..... Which is great on the 1 hand, since this plugin is not flawed 🎉
Otoh... @elchininet could I ask you to please have a look at the new feature  and the resource js file ?
it is a miniature of the former custom-ui that never caused anything like this, so maybe I made an error after all there.
Closing this issue here

but its not working for me :(

Disabling that immediately returns the correct functionality to Kiosk-mode.....
Which is great on the 1 hand, since this plugin is not flawed 🎉

Glad to hear that you found the issue.

Otoh...
@elchininet could I ask you to please have a look at the new feature and the resource js file ?
it is a miniature of the former custom-ui that never caused anything like this, so maybe I made an error after all there.

I'll take a look to see if I spot something but custom-ui was used by several people, Iam sure that the issue is not coming from there.

I can confirm that
kiosk_mode:
  kiosk: true

works but following doesn't
kiosk_mode:
  entity_settings:
    - entity: input_select.theme_selector
      state: ios-dark-mod
      hide_sidebar: true
      hide_header: true

That makes total sense because the first is a good configuration and the second is not correct.
Check the examples in the documentation for the entity_settings. The indentation is not right and it should not have a state field.
This should work:
kiosk_mode:
  entity_settings:
    - entity:
        input_select.theme_selector: 'ios-dark-mod'
      hide_sidebar: true
      hide_header: true

Let me know if it works and if it is not working for you open a new issue just to keep track of each one separately.
Regards

Ok, I can confirm the same findings. Checked in FireFox, Edge and Opera broswers, the input select doesn't work but the input_boolean does. Here is the code:
kiosk_mode:
  admin_settings:
    hide_sidebar: false
    hide_header: false
  mobile_settings:
    hide_header: false
    hide_sidebar: true
    ignore_entity_settings: true
    custom_width: 768
  user_settings:
    - users:
        - Test
      hide_sidebar: false
      hide_header: false
  entity_settings:
    - entity:
        input_select.theme_selector: ios-dark-mod
      hide_sidebar: true
      hide_header: true
    - entity:
        input_boolean.hide_sidebar: 'on'
      hide_sidebar: true
      hide_header: true
views:
  - title: Home
    cards:
      - type: entities
        entities:
          - input_select.theme_selector
      - type: entities
        entities:
          - input_boolean.hide_sidebar

One more point that I've automation for switching themes, is this causing an issue as the input_select change the theme but no effect for kiosk settings.
alias: Switch Theme
description: ""
trigger:
  - platform: state
    entity_id: input_select.theme_selector
action:
  - data_template:
      name: "{{ states('input_select.theme_selector') }}"
    action: frontend.set_theme

Hi @ausfas, please, open a new issue with the content of your previous post. This one is not related to the issue that you are having.

oops the silliest mistake
input_select.theme_selector: ios-dark-mod

should have been
input_select.theme_selector: ios-dark-mode

May be too much playing with card_mod and browser_mod.
All good and thanks for your patience and assistance.

oops the silliest mistake
input_select.theme_selector: ios-dark-mod

should have been
input_select.theme_selector: ios-dark-mode

May be too much playing with card_mod and browser_mod.
All good and thanks for your patience and assistance.

Glad to hear that it is solved for you 👍


Broken link

There is a broken link in the part Setting the TableType to Temporary that has a link to Performance of BACPAC generation https://docs.microsoft.com/en-us/performance/performance-onprem.md#performance-of-bacpac-generation.

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 19d3aca9-8d36-d509-86b5-01c304b39a60
Version Independent ID: 2beaf080-7fd7-569c-9738-c7158ac7dd8c
Content: Temporary Tables - Business Central
Content Source: dev-itpro/developer/devenv-temporary-tables.md
Service: dynamics365-business-central
GitHub Login: @blrobl
Microsoft Alias: solsen

Thanks for catching this. Will be fixed with the next doc refresh.


Consider enabling CKAN support in SpaceDock

According to the CKAN docs, there's a checkbox you can click in SpaceDock to automatically register the mod with their system. Alternatively the mod can be added manually.
CKAN is great! :)

I've enabled it now through spacedock. not very familiar with how CKAN works, so hopefully if there are any issues people can raise them in here.


Install instructions in README.md

Install instructions in README.md will install the original ol-geocoder:
npm install ol-geocoder
Instead should read:
npm install @kirtandesai/ol-geocoder

Thanks!


[BUG] SKSurface.ReadPixels is super slow

Description
I'm using SkiaSharp with a OpenGL backend and I'm drawing on a SkSurface wich is super fast, took around 0.25ms.
To get the rendered pixel data I'm using SkSurface.ReadPixels to copy the data to a buffer. But this calls are super slow ~30ms.
Is there any chance to speed it up? Or getting a direct pointer to the pixeldata?
Code
var _info = new SKImageInfo(1920, 1080, SKColorType.Bgra8888);
var _glContext = GlContext.Create();
var _glContext.MakeCurrent();
var _glInterface = GRGlInterface.Create();
var _context = GRContext.CreateGl(_glInterface);
var _buffer = Marshal.AllocHGlobal(_info.BytesSize);
_surface = SKSurface.Create(_context, true, _info);
// doing the actual drawing like _surface.Canvas.Draw...
_surface.ReadPixels(_info, _buffer, _info.RowBytes, 0, 0));
Basic Information

Version with issue: 2.80.0-preview.24
IDE: Rider 2020.1
Platform Target Frameworks: .Net Core 3.1

macOS 10.15.5 (currently only tested here)

I'm not an offical Skia developer, but here's my take on it.

you are drawing using a GPU backend. That allows for very fasting rendering.

however, when you read the pixels back to the CPU, all pixels must be transferred over the PCI bus from GPU memory to host CPU memory. This is called "readback", and is always slow. Furthermore, it stops both the GPU and CPU.

for high performance readback, async DMA must be used, so that neither GPU nor CPU are blocked while the pixels are being transferred. This is still slow, but at least none of the machinery will stall, so you can overlap rendering and readback. But I don't think SkiaSharp supports such an interface (that being said, Skia does have a protected asyncReadPixels and a public  GrSurfaceContext::asyncRescaleAndReadPixels; the latter could be exposed by the SkiaSharp 2.0 with some work)

I understand that the transfer is the problem, but I don't understand why.
When I calculate the amount auf data that should be transferred between GPU to RAM it's about 500 MB/s which for my knowledge is much less the actual speed capabilities of modern RAM or PCI busses.

Do you have a project that you can share? I am interested in profiling this on my system.

I created a minimal console application example. To make it work include the files from https://github.com/mono/SkiaSharp/tree/master/tests/Tests/GlContexts to the project.
var watch = new Stopwatch();
watch.Restart();

// See https://github.com/mono/SkiaSharp/tree/master/tests/Tests/GlContexts for context creation
var glContext = GlContext.Create();
glContext.MakeCurrent();

Console.WriteLine($"{watch.Elapsed.TotalMilliseconds:0.000}ms for OpenGl context creation");
watch.Restart();

var glInterface = GRGlInterface.Create();
var context = GRContext.CreateGl(glInterface);

Console.WriteLine($"{watch.Elapsed.TotalMilliseconds:0.000}ms for Skia context creation");
watch.Restart();

var info = new SKImageInfo(1920, 1080, SKColorType.Bgra8888);
var surface = SKSurface.Create(context, true, info);

Console.WriteLine($"{watch.Elapsed.TotalMilliseconds:0.000}ms for Skia surface creation");
watch.Restart();

var buffer = Marshal.AllocHGlobal(info.BytesSize);

Console.WriteLine($"{watch.Elapsed.TotalMilliseconds:0.000}ms for buffer allocation");

var totalDrawingTime = 0.0;
var totalCopyTime = 0.0;
var cycles = 10;

using (var paint = new SKPaint())
{
    paint.IsAntialias = true;
    paint.Color = SKColors.Red;
    paint.Style = SKPaintStyle.Stroke;
    paint.StrokeWidth = 4;

    for (var i = 0; i < cycles; i++)
    {
        Console.WriteLine($"## Cycle {i} ##");
        var rand = new Random(cycles);

        watch.Restart();

        // Drawing start
        surface.Canvas.Clear(SKColors.SkyBlue);

        for (var j = 0; j < 1000; j++)
        {
            surface.Canvas.DrawLine(rand.Next(0,info.Width), rand.Next(0,info.Height), rand.Next(0,info.Width), rand.Next(0,info.Height), paint);
        }
        // Drawing end

        totalDrawingTime += watch.Elapsed.TotalMilliseconds;
        Console.WriteLine($"  {watch.Elapsed.TotalMilliseconds:0.000}ms for drawing");
        watch.Restart();

        if (!surface.ReadPixels(info, buffer, info.RowBytes, 0, 0))
        {
            Console.WriteLine($"Failed to copy pixels from GPU");
        }

        totalCopyTime += watch.Elapsed.TotalMilliseconds;
        Console.WriteLine($"  {watch.Elapsed.TotalMilliseconds:0.000}ms for copying from GPU");

    }

}

watch.Restart();

surface.Dispose();
Marshal.FreeHGlobal(buffer);
context.Dispose();
glContext.Destroy();

Console.WriteLine($"{watch.Elapsed.TotalMilliseconds:0.000}ms for freeing up");
watch.Restart();

Console.WriteLine($"Average of {totalDrawingTime / cycles:0.000}ms for drawing");
Console.WriteLine($"Average of  {totalCopyTime/ cycles:0.000}ms for copying from GPU");

On my mac I get the following output (no Debugger attached, Release configuration, build for x64):
46.406ms for OpenGl context creation
588.590ms for Skia context creation
1.772ms for Skia surface creation
0.192ms for buffer allocation
## Cycle 0 ##
  9.028ms for drawing
  71.424ms for copying from GPU
## Cycle 1 ##
  2.682ms for drawing
  55.342ms for copying from GPU
## Cycle 2 ##
  1.817ms for drawing
  33.128ms for copying from GPU
## Cycle 3 ##
  2.244ms for drawing
  65.454ms for copying from GPU
## Cycle 4 ##
  1.941ms for drawing
  34.987ms for copying from GPU
## Cycle 5 ##
  1.690ms for drawing
  35.023ms for copying from GPU
## Cycle 6 ##
  1.894ms for drawing
  47.286ms for copying from GPU
## Cycle 7 ##
  1.738ms for drawing
  34.455ms for copying from GPU
## Cycle 8 ##
  1.915ms for drawing
  32.325ms for copying from GPU
## Cycle 9 ##
  1.853ms for drawing
  44.672ms for copying from GPU
23.287ms for freeing up
Average of 2.676ms for drawing
Average of  45.408ms for copying from GPU

Could you add surface.Canvas.Flush() and/or context.Flush() just after // Drawing end, and measure again?
I think Skia might batch many commands before sending them to the GPU. So the 44ms you are measuring might also include a subset of the 1000 lines you are drawing?

With surface.Canvas.Flush()
Average of 4.445ms for drawing
Average of  62.336ms for copying from GPU

With context.Flush()
Average of 3.796ms for drawing
Average of  57.863ms for copying from GPU

With surface.Canvas.Flush() and context.Flush()
Average of 3.619ms for drawing
Average of  58.371ms for copying from GPU

And without any change (it's a little bit slower now, so added for reference)
Average of 3.174ms for drawing
Average of  52.344ms for copying from GPU

Okay, I'll try to reproduce your test case on Monday; I'm curious what is causing this, and what can be done about it.

@Ziriax did you had time to look into this?

Sorry, I completely forgot about this. I'll try to find some time.

I got this running on my machine (Windows 10 x64, RTX 2070, AMD Ryzen 3950x), I get the following numbers with your code:
305,354ms for OpenGl context creation
35,677ms for Skia context creation
0,990ms for Skia surface creation
0,174ms for buffer allocation
## Cycle 0 ##
  5,529ms for drawing
  14,434ms for copying from GPU
## Cycle 1 ##
  1,463ms for drawing
  11,229ms for copying from GPU
## Cycle 2 ##
  1,475ms for drawing
  7,258ms for copying from GPU
## Cycle 3 ##
  1,442ms for drawing
  7,039ms for copying from GPU
## Cycle 4 ##
  1,464ms for drawing
  7,973ms for copying from GPU
## Cycle 5 ##
  1,422ms for drawing
  7,358ms for copying from GPU
## Cycle 6 ##
  1,410ms for drawing
  7,347ms for copying from GPU
## Cycle 7 ##
  1,443ms for drawing
  6,955ms for copying from GPU
## Cycle 8 ##
  1,441ms for drawing
  6,988ms for copying from GPU
## Cycle 9 ##
  1,452ms for drawing
  6,958ms for copying from GPU
6,365ms for freeing up
Average of 1,854ms for drawing
Average of  8,354ms for copying from GPU

I'll try again with a debug build of Skia, to see what is taking time.

Is that 8ms or 8000ms?

My locale settings seem to use Dutch, so the 8,354ms is actually 8.354ms :)
So 8ms

As expected, glReadPixels is called, and that is a synchronous call, and stalls the pipeline.
However, just after that, the pixels are converted... It seems the orientation if different, and the pixels need to be flipped. I'm trying to figure out what happens.

@mattleibow I found a silly bug in SkiaSharp, in
SkiaSharp\binding\Binding\SKSurface.cs
		public static SKSurface Create (GRContext context, bool budgeted, SKImageInfo info, int sampleCount, GRSurfaceOrigin origin) =>
			Create (context, budgeted, info, sampleCount, GRSurfaceOrigin.BottomLeft, null, false);

As you can see the origin is not passed to the Create function, GRSurfaceOrigin.BottomLeft is passed.
This causes the pixel flip.
I will patch this, and see what happens :)

@joa77 Could you change you code in
var surface = SKSurface.Create(context, true, info, 0, GRSurfaceOrigin.TopLeft, new SKSurfaceProperties(SKPixelGeometry.Unknown), false);

And profile again?

In my debug build, this reduces the readpixels from 8ms to 4ms...
Going to try a release build now... I'm be back in 2 hours, LOL

@mattleibow  I see GRSurfaceOrigin.BottomLeft is always passed as the default for the origin. Is this for a reason? Because the native read-pixels code is:
    bool flip = srcProxy->origin() == kBottomLeft_GrSurfaceOrigin;

    auto supportedRead = caps->supportedReadPixelsColorType(
            this->colorInfo().colorType(), srcProxy->backendFormat(), dstInfo.colorType());

    bool makeTight = !caps->readPixelsRowBytesSupport() && tightRowBytes != rowBytes;

    bool convert = unpremul || premul || needColorConversion || flip || makeTight ||
                   (dstInfo.colorType() != supportedRead.fColorType);

So it seems to prefer a TopLeft origin, otherwise it will convert the pixels.

The origin was BL for historical reasons I think. Here is the origin in m60:
https://github.com/google/skia/blob/chrome/m60/include/core/SkSurface.h#L167-L168
I don't know about changing the default, but how does this affect different platforms? do iOS/macOS and Linux and Windows do different things? It may have to do with the fact that it was originally meant to match the GL origin of bottom left: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glReadPixels.xhtml

In a release build, when passing TopLeft as the surface origin, I get the same numbers.
So almost twice as fast on my machine!
Skia also has an asyncReadPixels, but this isn't exposed yet in SkiaSharp (I even don't think it is part of the public Skia API).
306,579ms for OpenGl context creation
8,506ms for Skia context creation
0,856ms for Skia surface creation
0,151ms for buffer allocation
## Cycle 0 ##
  10,819ms for drawing
  5,958ms for copying from GPU
## Cycle 1 ##
  1,597ms for drawing
  7,008ms for copying from GPU
## Cycle 2 ##
  1,549ms for drawing
  4,031ms for copying from GPU
## Cycle 3 ##
  1,546ms for drawing
  3,984ms for copying from GPU
## Cycle 4 ##
  1,555ms for drawing
  4,236ms for copying from GPU
## Cycle 5 ##
  1,565ms for drawing
  3,853ms for copying from GPU
## Cycle 6 ##
  1,539ms for drawing
  4,028ms for copying from GPU
## Cycle 7 ##
  1,574ms for drawing
  3,772ms for copying from GPU
## Cycle 8 ##
  1,504ms for drawing
  3,893ms for copying from GPU
## Cycle 9 ##
  1,590ms for drawing
  3,849ms for copying from GPU
6,173ms for freeing up
Average of 2,484ms for drawing
Average of  4,461ms for copying from GPU

@joa77 Could you change you code in
var surface = SKSurface.Create(context, true, info, 0, GRSurfaceOrigin.TopLeft, new SKSurfaceProperties(SKPixelGeometry.Unknown), false);

And profile again?

Do i need to get a new build of SkiaSharp to do this?

Do i need to get a new build of SkiaSharp to do this?

No this particular overload works fine.

It's around 4ms faster now (running on macOS, maybe this makes a difference depending on the OS)
Any chance that the asyncReadPixels method will get implemented in SkiaSharp?

Okay, since the conversion is done on the CPU, this explains why we both get 4ms when the conversion isn't done.
In my case, the glReadPixels call takes 4ms. On your machine, it takes about 40ms. This is really odd. Are you sure the videocard is in a high bandwidth PCI slot? Because 40ms is way too high.
You might want to profile your OpenGL calls, just to make sure. I haven't done that for a while, but maybe tools like https://apitrace.github.io/ and https://renderdoc.org/ can help

In the old days, we used two PBO surfaces, then glReadPixels becomes async "for free", e.g. without callbacks:
http://www.songho.ca/opengl/gl_pbo.html
You might be able to use that directly, without calling Skia's read pixels, not sure

I'm facing the same problem. Also running on macOS.
I tried the benchmark code found gethere results of around 25ms for copying from GPU to CPU.
Are there any chances of improvement?

I'm facing the same problem. Also running on macOS.
I tried the benchmark code found gethere results of around 25ms for copying from GPU to CPU.
Are there any chances of improvement?

@joa77 Could you change you code in

var surface = SKSurface.Create(context, true, info, 0, GRSurfaceOrigin.TopLeft, new SKSurfaceProperties(SKPixelGeometry.Unknown), false);

And profile again?

Can you try this code and see?

@joa77 Could you change you code in

var surface = SKSurface.Create(context, true, info, 0, GRSurfaceOrigin.TopLeft, new SKSurfaceProperties(SKPixelGeometry.Unknown), false);

And profile again?

Can you try this code and see?

Sorry for the late response, when trying this code I'm getting an improvement of ~4ms


Added Header Text and style

Added Header Text 'Hello SHEros' and Styled it with blue color.

Hi @Shafaaa321 ,
Good job so far!
You still need to work on some issues to get your project approved, but you are almost there!

To Highlight 💯

Clean code 👍

Required Changes ♻

Please make your commit to be a minimum of 3 for best practices

Cheers and Happy coding! 👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.


tabBar to position Top issue

Hi,
On android when using:
steroids.tabBar.update({       position: 'top'     }); it is pushing the super-nav down.
That would be fine if we could put the navigationBar to Top as well (eg. steroids.navigationBar.update({       position: 'top'     });).
That is not working so there is no way to reorder thoes.

Sounds like a legit bug, thanks for reporting!


Add support for embedded teaming

Fixes #95
Added support for the embedded teaming option that is new in server 2016.
@kwirkykat @mbreakey3 Do you guys know who is able to review stuff for this module? I don't see anyone on the maintainers list.

This change is

@BrianFarnhill,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@BrianFarnhill It looks like this functionality has been earmarked to get merged into the main xNetworking module from @rchaganti 's cHyper-V module. It's probably having a chat with Ravi and @PlagueHO to ensure that there's no duplication or additional waste of effort here.

I am waiting on completing all unit/integration tests. But, if this PR has all needed ingredients, we should be OK.

@BrianFarnhill if it's OK with you, I will merge my cVMSwitch as xVMSwitch here. My module has localized messages and other changes needed for making this a HQRM resource. I am trying to see if we can avoid any rework on this.

@iainbrighton Why would we put this in to xNetworking though? This is the hyper-v specific implementation of the teaming which is different to the standard network teaming (well not under the hood, but in terms of how it's managed at least). Given the management of it is handled by hyper-v with the hyper-v commandlets when you do the teaming like this, I would argue that this belongs to the Hyper-V module. Would love to get @PlagueHO thoughts on this though.
@rchaganti Happy for you to put a pull request to my fork with the changes you have so they can get included in this PR. I was going to do another run over this whole module for the HQRM standards when I got some time as well so if you have some work already started there that would be great!

So, @BrianFarnhill I reviewed the differences (your PR vs what I have in cVMSwitch) in the module code and see that you have a subset of what I am doing.  That way, you don't have to update this PR. Makes sense?

@rchaganti Sure - so who is reviewing and merging my PR here then?

That would be @bgelens who is maintainer for this module. He will come around here soon I imagine.

There is support in xNetworking for teaming adapters, but as @BrianFarnhill says, this is a different thing that enabling teams on a Hyper-V VM Switch. So it definitely makes more sense to be in here.
I think @bgelens is starting to catch up on all this stuff now.

@PlagueHO I meant that @rchaganti had earmarked/mentioned that he "was looking to merge" his xVMSwitch implementation into the xNetworking module - not that this has been done. I just wanted to make sure there was no duplicated effort! It sounds like @BrianFarnhill and Ravi now have it under control... 👀

@iainbrighton - Oh right! Doh! My lack of comprehension there. But yes, I don't think xNetworking is the place right for xVMSwitch - xHyper-V is definitely where people are going to expect to find it (at least that is where I'd look :grin: )

@BrianFarnhill I'll look at this as soon as I can :) I just started maintaining a couple of the repos and am trying to get my grips on the backlog on all of them.

@BrianFarnhill Thanks for your hard work! I finally had some time to review. So sorry for the delay (there is a lot of backlog to deal with in this repo).
Please review / address my comment. I am a bit worried about the resource itself though. It doesn't seem to be a solid reliable one and maybe needs an overhaul / redesign which is why I pinged some others in one of the comments to have some additional opinions.

Reviewed 2 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

README.md, line 67 at r2 (raw file):

* **Name**: The desired VM Switch name
* **Type**: The desired type of switch: { External | Internal | Private }
* **NetAdapterName**: Network adapter name/s for external switch type

Could you change name/s to name(s)

README.md, line 71 at r2 (raw file):

* **EnableEmbeddedTeaming**: Should embedded NIC teaming be used (Windows Server 2016 only)
* **BandwidthReservationMode**: Specify the QoS mode used (options other than NA are only supported on Hyper-V 2012+): { Default | Weight | Absolute | None | NA }.
* **Ensure**: Ensures that the VM Switch is Present or Absent

Would you mind updating all of these under xVMSwitch so they state the data type and dsc property type (as with xVMDvdDrive)?
e.g:

[Boolean] EnableEmbeddedTeaming (Write): Specifies switch embedded NIC teaming should be used (Windows Server 2016 only)

README.md, line 105 at r2 (raw file):

* Fix bug in xVMDvdDrive with hardcoded VM Name.
* Corrected Markdown rule violations in Readme.md.
* Added support for Embedded NIC teaming in Server 2016 to xVMSwitch

please rephrase:
MSFT_xVMHyperV: Added support for Switch Embedded Teaming (SET) in Server 2016

README.md, line 771 at r2 (raw file):

            Ensure = 'Present'
            Name   = 'Hyper-V'
        }

The Hyper-V PowerShell module needs to be installed as well or the xHyper-V resources won't work. Please add another WindowsFeature resource to install this

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 31 at r2 (raw file):

    {
        $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
        $description = $switch.NetAdapterInterfaceDescriptions

I'm not too font on this construct as it's really hard to see the diff being a plural against a singular. Could you make this more elegant?
Isn't it so that the Plural property is only populated when it's a SET switch?
You can make it something like:
if ($switch.EmbeddedTeaminEnabled)
{
    # when SET, use NetAdapterInterfaceDescriptions (plural)
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
    $description = $switch.NetAdapterInterfaceDescriptions
}
else
{
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescription -ErrorAction SilentlyContinue).Name
    $description = $switch.NetAdapterInterfaceDescription
}

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 46 at r2 (raw file):

    if($switch.BandwidthReservationMode -ne $null)
    {
        $returnValue['BandwidthReservationMode'] = $switch.BandwidthReservationMode

I know this is not code from your PR but please update so BandwidthReservationMode is always part of the hashtable. All properties in the schema should be accounted for and no more as defined in the schema may exist.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 91 at r2 (raw file):

    if ($EnableEmbeddedTeaming -eq $true -and (Get-OSVersion).Major -lt 10)
    {
        throw "Embedded teaming is only supported on Windows Server 2016"

Please make use of New-InvalidArgumentError from HyperVCommon  instead of throw. Doing this makes sure we can add tests for the specific error.
For an example see: https://github.com/PowerShell/xHyper-V/blob/fabd28b7f5292cf1e36d7e0a572c06683f40541e/DSCResources/MSFT_xVMDvdDrive/MSFT_xVMDvdDrive.psm1#L399

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 112 at r2 (raw file):

                }
            }
            else

remove trailing whitespace behind else

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 132 at r2 (raw file):

            {
                Write-Verbose -Message "The switch $Name EnableEmbeddedTeaming is incorrect ..."
                $removeReaddSwitch = $true

any idea why this is called removeReaddSwitch all over the place? Could you make an effort to rename these to removeReadSwitch?

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

    }

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Why do we have 2 ways of detecting the OS version now? Can you update so only one is used (I prefer Get-WmiObject to be replaced)

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 273 at r2 (raw file):

    if ($EnableEmbeddedTeaming -eq $true -and (Get-OSVersion).Major -lt 10)
    {
        throw "Embedded teaming is only supported on Windows Server 2016"

Please make use of New-InvalidArgumentError from HyperVCommon  instead of throw. Doing this makes sure we can add tests for the specific error.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 346 at r2 (raw file):

                        else
                        {
                            Write-Verbose -Message "Switch $Name has a correct list of network adapters"

so if $null -eq $switch.NetAdapterInterfaceDescriptions all is good? With embedded teaming it's not possible to have no NetAdapterInterfaceDescriptions I think. If this is an impossible situation, the if / else construct can be removed, the content from the else scriptblock can be removed and the content of the if scriptblock can be moved to the parent else.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 371 at r2 (raw file):

                # Only check embedded teaming if specified
                if ($PSBoundParameters.ContainsKey("EnableEmbeddedTeaming") -eq $true)

It is possible this construct is never called  due to the return $true right after the if ($PSBoundParameters.ContainsKey("AllowManagementOS"))   construct.
So if AllowManagementOS is not provided, the logic ends there.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 376 at r2 (raw file):

                    if($switch.EmbeddedTeamingEnabled -eq $EnableEmbeddedTeaming -or $null -eq $switch.EmbeddedTeamingEnabled)
                    {
                        Write-Verbose -Message "Switch $Name has correct EnableEmbeddedTeaming or it does not apply to this OS"

this is the final call in the if ($Ensure -eq 'Present') where is the true when this complies?
I think this resource needs some general redesign pinging @PlagueHO, @iainbrighton and @rchaganti for input

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):

<#
.SYNOPSIS
Returns the OS version, it is put here so we can mock it for tests only though

but it's used in the code. So not only for mocks right?

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 1 at r2 (raw file):

[CmdletBinding()]

please make use of the test templates
for the guideline on how to use these see TestsGuidelines

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 159 at r2 (raw file):

        }

        Context "A virtual switch with embedded teaming does not exist but should" {

Missing a test for OS version lower then required and an error should be thrown

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 181 at r2 (raw file):

            it "Should run the set method without exceptions" {
                Set-TargetResource @testParams

Missing test like Assert Get-VMSwitch and New-VMSwitch is called exactly 1 time, etc. This will make sure which code path has been used.
Example if Get-VMSwitch was called twice, the code path for an existing switch is followed which is something we don't want. So to spot regression it is good to add these.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 210 at r2 (raw file):

            it "Should return true in the test method" {
                Test-TargetResource @testParams | Should Be $true

I'm pretty sure this is the $true from if ($PSBoundParameters.ContainsKey("AllowManagementOS")) {

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 256 at r2 (raw file):

            it "Should run the set method without exceptions" {
                Set-TargetResource @testParams

Missing the assert called tests (won't repeat for the rest)

Comments from Reviewable

Sorry about all the style nitpicks - but I figure as we'll want to bring this up to HQRM then we should be knocking these out now rather than later. I didn't list them all but the ones I mentioned usually occur more than once. Great job on the feature though - it will be awesome!

Reviewed 2 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 60 unresolved discussions.

README.md, line 746 at r2 (raw file):

This configuration will create an external VM Switch that uses multiple NICs
and embedded teaming, on Hyper-V host that runs Server 2016

nitpick: ...teaming, on a Hyper-V...

README.md, line 753 at r2 (raw file):

    param
    (
        [string[]]$NodeName = 'localhost',

Move $NodeName = 'localhost' to next line and add [Parameter()]

README.md, line 755 at r2 (raw file):

        [string[]]$NodeName = 'localhost',

        [Parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

README.md, line 756 at r2 (raw file):

        [Parameter(Mandatory)]
        [string]$SwitchName,

Move $SwitchName to next line

README.md, line 758 at r2 (raw file):

        [string]$SwitchName,

        [Parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

README.md, line 759 at r2 (raw file):

        [Parameter(Mandatory)]
        [string[]]$NetAdapterNames

Move $NetAdapterNames to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 39 at r2 (raw file):

        AllowManagementOS     = $switch.AllowManagementOS
        EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
        Ensure                = if($switch){'Present'}else{'Absent'}

If only we had a ternary operator in PS but should be converted to:
$Ensure = if($switch)
{
'Present'
}
else{
'Absent'
}

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 44 at r2 (raw file):

    }

    if($switch.BandwidthReservationMode -ne $null)

Space after if and $null on left side of comparison.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 58 at r2 (raw file):

    param
    (
        [parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 59 at r2 (raw file):

    (
        [parameter(Mandatory)]
        [String]$Name,

Move $Name to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 61 at r2 (raw file):

        [String]$Name,

        [parameter(Mandatory)]

Change to [Parameter(Mandatory = $true)]

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 63 at r2 (raw file):

        [parameter(Mandatory)]
        [ValidateSet("External","Internal","Private")]
        [String]$Type,

Move $Type to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 65 at r2 (raw file):

        [String]$Type,

        [ValidateNotNullOrEmpty()]

Add [Parameter()] above

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 66 at r2 (raw file):

        [ValidateNotNullOrEmpty()]
        [String[]]$NetAdapterName,

Move $NetAdapterName to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 68 at r2 (raw file):

        [String[]]$NetAdapterName,

        [Boolean]$AllowManagementOS = $false,

Add [Parameter()] above

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

        [Boolean]$AllowManagementOS = $false,

        [Boolean]$EnableEmbeddedTeaming = $false,

Add [Parameter()] above

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):

        [Boolean]$AllowManagementOS = $false,

        [Boolean]$EnableEmbeddedTeaming = $false,

Move $EnableEmbeddedTeaming to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 72 at r2 (raw file):

        [Boolean]$EnableEmbeddedTeaming = $false,

        [ValidateSet("Default","Weight","Absolute","None","NA")]

Add [Parameter()] above

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 73 at r2 (raw file):

        [ValidateSet("Default","Weight","Absolute","None","NA")]
        [String]$BandwidthReservationMode = "NA",

Move $BandwidthReservationMode to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 75 at r2 (raw file):

        [String]$BandwidthReservationMode = "NA",

        [ValidateSet("Present","Absent")]

Add [Parameter()] above

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r2 (raw file):

        [ValidateSet("Present","Absent")]
        [String]$Ensure = "Present"

Move $Ensure to next line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 79 at r2 (raw file):

    )
    # Check if Hyper-V module is present for Hyper-V cmdlets
    if(!(Get-Module -ListAvailable -Name Hyper-V))

Add space after if - also appears elsewhere - suggest search/replace.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 81 at r2 (raw file):

    if(!(Get-Module -ListAvailable -Name Hyper-V))
    {
        Throw "Please ensure that the Hyper-V role is installed with its PowerShell module"

See @bgelen's comment about throw further down

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 83 at r2 (raw file):

        Throw "Please ensure that the Hyper-V role is installed with its PowerShell module"
    }
    # Check to see if the BandwidthReservationMode chosen is supported in the OS

Add blank line after end of if block - also appears in other places. Please see styleguidelines.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 84 at r2 (raw file):

    }
    # Check to see if the BandwidthReservationMode chosen is supported in the OS
    elseif(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Add space after elseif - again, suggest search replace.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 86 at r2 (raw file):

    elseif(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))
    {
        Throw "The BandwidthReservationMode cannot be set on a Hyper-V version lower than 2012"

See @bgelen's comment about throw further down

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):

            }

            if(($BandwidthReservationMode -ne "NA") -and ($switch.BandwidthReservationMode -ne $BandwidthReservationMode))

Please add space after if.
Also, convert to single quotes around NA.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 135 at r2 (raw file):

            }

            if($removeReaddSwitch)

Please add space after if

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 140 at r2 (raw file):

                $switch | Remove-VMSwitch -Force
                $parameters = @{}
                $parameters["Name"] = $Name

Please convert to single quotes - this needs to be done all over.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 146 at r2 (raw file):

                    $parameters["MinimumBandwidthMode"] = $BandwidthReservationMode
                }
                if($PSBoundParameters.ContainsKey("AllowManagementOS"))

Add blank line after if block

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 150 at r2 (raw file):

                    $parameters["AllowManagementOS"] = $AllowManagementOS
                }
                if($PSBoundParameters.ContainsKey("EnableEmbeddedTeaming"))

Add blank line after if block

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 154 at r2 (raw file):

                    $parameters["EnableEmbeddedTeaming"] = $EnableEmbeddedTeaming
                }
                $null = New-VMSwitch @parameters

Add blank line after if block

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 226 at r2 (raw file):

    param
    (
        [parameter(Mandatory)]

Can this parameter block please be fixed as per previous comments?

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):

    }

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))

Suggest using the OS detection method from PSDscResources and add the function to the common helper module.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 268 at r2 (raw file):

    if(($BandwidthReservationMode -ne "NA") -and ([version](Get-WmiObject -Class 'Win32_OperatingSystem').Version -lt [version]'6.2.0'))
    {
        Throw "The BandwidthReservationMode cannot be set on a Hyper-V version lower than 2012"

Throw using commo

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):

                        Write-Verbose -Message "Checking if Switch $Name has correct NetAdapterInterface ..."
                        $adapter = $null
                        try

This try catch block seems to serve no purpose. The SilentlyContinue prevents it from ever hitting the catch block anyway.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 402 at r2 (raw file):

}

<#

This should be moved to the Helper functions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 8 at r2 (raw file):

    [Write, Description("Network adapter name/s for external switch type")] String NetAdapterName[];
    [Write, Description("Specify if the VM host has access to the physical NIC")] Boolean AllowManagementOS;
    [Write, Description("Support embedded teaming on this switch? (Server 2016 only)")] Boolean EnableEmbeddedTeaming;

Text should match readme parameter.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 78 at r2 (raw file):

            )
        }

Please remove extra blank lines here.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 172 at r2 (raw file):

            }

            it "Should return absent in the get method" {

Use capital I for it - suggest using search replace to fix all.

Comments from Reviewable

@PlagueHO You're killing me dude, I was just following the style of the existing code because I didn't wanna go through and touch all the bits that weren't mine :P I'll see what I can do with it though!

Sorry @BrianFarnhill :grin: there is no need to do the HQRM stuff for the code not changed in the PR - we can do this at a later date (it'll happen sometime). I'm just in the habit of looking at the whole file (thanks @johlju :stuck_out_tongue_winking_eye: ).  I definitely won't hold this PR up for that. So just do what you have time for.

Review status: 3 of 6 files reviewed at latest revision, 60 unresolved discussions, some commit checks failed.

README.md, line 67 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Could you change name/s to name(s)

Done.

README.md, line 71 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Would you mind updating all of these under xVMSwitch so they state the data type and dsc property type (as with xVMDvdDrive)?
e.g:

[Boolean] EnableEmbeddedTeaming (Write): Specifies switch embedded NIC teaming should be used (Windows Server 2016 only)

Done.

README.md, line 105 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
please rephrase:
MSFT_xVMHyperV: Added support for Switch Embedded Teaming (SET) in Server 2016

Done.

README.md, line 771 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
The Hyper-V PowerShell module needs to be installed as well or the xHyper-V resources won't work. Please add another WindowsFeature resource to install this

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 31 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
I'm not too font on this construct as it's really hard to see the diff being a plural against a singular. Could you make this more elegant?
Isn't it so that the Plural property is only populated when it's a SET switch?
You can make it something like:
if ($switch.EmbeddedTeaminEnabled)
{
    # when SET, use NetAdapterInterfaceDescriptions (plural)
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescriptions).Name
    $description = $switch.NetAdapterInterfaceDescriptions
}
else
{
    $netAdapterName = (Get-NetAdapter -InterfaceDescription $switch.NetAdapterInterfaceDescription -ErrorAction SilentlyContinue).Name
    $description = $switch.NetAdapterInterfaceDescription
}

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 46 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
I know this is not code from your PR but please update so BandwidthReservationMode is always part of the hashtable. All properties in the schema should be accounted for and no more as defined in the schema may exist.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 91 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Please make use of New-InvalidArgumentError from HyperVCommon  instead of throw. Doing this makes sure we can add tests for the specific error.
For an example see: https://github.com/PowerShell/xHyper-V/blob/fabd28b7f5292cf1e36d7e0a572c06683f40541e/DSCResources/MSFT_xVMDvdDrive/MSFT_xVMDvdDrive.psm1#L399

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 112 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
remove trailing whitespace behind else

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 132 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
any idea why this is called removeReaddSwitch all over the place? Could you make an effort to rename these to removeReadSwitch?

My best guess here is that it's been abbreviated from "Remove and re-add" (thus RemoveReadd and not an abbreviated for of "Remove Read"). So I don't think this needs to be changed.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 273 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Please make use of New-InvalidArgumentError from HyperVCommon  instead of throw. Doing this makes sure we can add tests for the specific error.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 346 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
so if $null -eq $switch.NetAdapterInterfaceDescriptions all is good? With embedded teaming it's not possible to have no NetAdapterInterfaceDescriptions I think. If this is an impossible situation, the if / else construct can be removed, the content from the else scriptblock can be removed and the content of the if scriptblock can be moved to the parent else.

I had the wrong wording on the verbose message here. Basically what we are testing for here is that if you desire embedded teaming to be enabled (checked at the outer if) that there should be a descriptions value (in the plural property) and if not, return false and say its wrong. So we were returning false but outputting the wrong text which is why I think this got confused, so I fixed that but left the logic as is.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 376 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
this is the final call in the if ($Ensure -eq 'Present') where is the true when this complies?
I think this resource needs some general redesign pinging @PlagueHO, @iainbrighton and @rchaganti for input

I rearranged the return $true statements to basically say "if I get to the end of it and haven't found a reason to return false that I'll just return true" so that should help the flow of things here I think.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
but it's used in the code. So not only for mocks right?

It is used in the code, but the only reason it exists as a cmdlet is so we can mock it. I can mock a cmdlet but I can't (easily) override a system type to mock it's value, so we drop it in this cmdlet for that reason. Happy to update the synopsis to make that clearer if you think it needs it.

Comments from Reviewable

Review status: 3 of 6 files reviewed at latest revision, 60 unresolved discussions, some commit checks failed.

README.md, line 746 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
nitpick: ...teaming, on a Hyper-V...

Done.

README.md, line 753 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $NodeName = 'localhost' to next line and add [Parameter()]

Done.

README.md, line 755 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Change to [Parameter(Mandatory = $true)]

Done.

README.md, line 756 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $SwitchName to next line

Done.

README.md, line 758 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Change to [Parameter(Mandatory = $true)]

Done.

README.md, line 759 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $NetAdapterNames to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 39 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
If only we had a ternary operator in PS but should be converted to:
$Ensure = if($switch)
{
'Present'
}
else{
'Absent'
}

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 44 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Space after if and $null on left side of comparison.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 58 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Change to [Parameter(Mandatory = $true)]

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 59 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $Name to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 61 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Change to [Parameter(Mandatory = $true)]

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 63 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $Type to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 65 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add [Parameter()] above

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 66 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $NetAdapterName to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 68 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add [Parameter()] above

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add [Parameter()] above

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 70 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $EnableEmbeddedTeaming to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 72 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add [Parameter()] above

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 73 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $BandwidthReservationMode to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 75 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add [Parameter()] above

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Move $Ensure to next line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 79 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add space after if - also appears elsewhere - suggest search/replace.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 81 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
See @bgelen's comment about throw further down

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 83 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line after end of if block - also appears in other places. Please see styleguidelines.

This one is an elseif, so I assumed that rule doesn't apply here. But will fix other instances

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 84 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add space after elseif - again, suggest search replace.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 86 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
See @bgelen's comment about throw further down

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Please add space after if.
Also, convert to single quotes around NA.

Am I missing something or is there nothing in the current version of the style guideline about the whole single vs double quotes thing. I know it technically makes a difference to processing, but are we seriously at the point where we are enforcing that sort of crap where it will in most cases make little to no difference?

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 135 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Please add space after if

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 140 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Please convert to single quotes - this needs to be done all over.

See above question on this style request

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 146 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line after if block

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 150 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line after if block

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 154 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line after if block

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 226 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can this parameter block please be fixed as per previous comments?

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Suggest using the OS detection method from PSDscResources and add the function to the common helper module.

What method are you meaning? A quick search and I found this - https://github.com/PowerShell/PSDscResources/blob/15b40ddabdf6a46e40fc17d0d07535cf12580c89/DscResources/MSFT_WindowsFeature/MSFT_WindowsFeature.psm1#L549 - but it's doing literally what we are doing, it's just not in a common helper thing in this case. So is the issue here that this should go in the helpers module but the approach is fine? I just wanted to check before processing this change

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Why do we have 2 ways of detecting the OS version now? Can you update so only one is used (I prefer Get-WmiObject to be replaced)

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 268 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Throw using commo

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This try catch block seems to serve no purpose. The SilentlyContinue prevents it from ever hitting the catch block anyway.

Not true, I was seeing scenarios where the SilentlyContinue was still seeing exceptions be thrown - I know it makes no sense and that it shouldn't behave like that, but it absolutely was which is why I had to include this to get it working

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 371 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
It is possible this construct is never called  due to the return $true right after the if ($PSBoundParameters.ContainsKey("AllowManagementOS"))   construct.
So if AllowManagementOS is not provided, the logic ends there.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 402 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This should be moved to the Helper functions.

Leaving this one for now based on the question above about the approach here

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 8 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Text should match readme parameter.

Done.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 60 unresolved discussions.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 1 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
please make use of the test templates
for the guideline on how to use these see TestsGuidelines

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 78 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Please remove extra blank lines here.

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 159 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Missing a test for OS version lower then required and an error should be thrown

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 172 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Use capital I for it - suggest using search replace to fix all.

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 181 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Missing test like Assert Get-VMSwitch and New-VMSwitch is called exactly 1 time, etc. This will make sure which code path has been used.
Example if Get-VMSwitch was called twice, the code path for an existing switch is followed which is something we don't want. So to spot regression it is good to add these.

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 210 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
I'm pretty sure this is the $true from if ($PSBoundParameters.ContainsKey("AllowManagementOS")) {

Done.

Tests/Unit/MSFT_xVMSwitch_EnableEmbeddedTeaming.Tests.ps1, line 256 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Missing the assert called tests (won't repeat for the rest)

Done.

Comments from Reviewable

@PlagueHO @bgelens OK I've put all of the code review feedback in to the latest updates on here, hopefully that should be enough to get it over the line.

Thanks, I did an initial review of some of it but don't have time to complete just now (also need to do my day job ;) ). I'll do the rest later today or tomorrow.

Reviewed 1 of 7 files at r4.
Review status: 1 of 6 files reviewed at latest revision, 56 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):
Previously, BrianFarnhill (Brian Farnhill) wrote…
Not true, I was seeing scenarios where the SilentlyContinue was still seeing exceptions be thrown - I know it makes no sense and that it shouldn't behave like that, but it absolutely was which is why I had to include this to get it working

OK I know about a lot of cmdlets not respecting ErrorAction and have created these kind of solutions for other work as well.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 23 at r4 (raw file):

    -Path (Split-Path -Path $PSScriptRoot -Parent) `
    -ChildPath '\HyperVCommon\HyperVCommon.psm1' )

There are 4 spaces here, could you remove them please so it's just a blank line

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):

    -ChildPath '\HyperVCommon\HyperVCommon.psm1' )

function Get-TargetResource

Could you add Synopsis and Parameter help for all functions
See functions-have-comment-based-help for guidance

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 71 at r4 (raw file):

    }

Could you reduce the amount of blank newlines to just 1 and make sure the newline isn't filled with spaces.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):

    else
    {
        $returnValue['BandwidthReservationMode'] = 'NA'

Thanks for adding this! Could you remove the 3 trailing spaces after 'NA' please

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):

                }

                New-VMSwitch @parameters | Out-Null

Could you revert these (I see a couple). $null =  is preferred over | Out-Null in general.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 341 at r4 (raw file):

            -ErrorMessage $LocalizedData.NetAdapterNameRequiredError
    }

could you remove the 1 excessive new line and also the spaces on the other

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 407 at r4 (raw file):

                        catch
                        {
                            Write-Verbose -Message "Network adapter not found"

These messages should also be localized as with the error messages.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 471 at r4 (raw file):

                }

                return $true

please remove trailing spaces

Comments from Reviewable

Sorry about the false negatives @BrianFarnhill - looking good to go from my perspective. I'm thinking we should get the new VS Code settings added to this repo sooner rather than later as it'll easily clean up many of these issues that had to be done manually.

Reviewed 5 of 7 files at r4.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 122 at r2 (raw file):
Previously, BrianFarnhill (Brian Farnhill) wrote…
Am I missing something or is there nothing in the current version of the style guideline about the whole single vs double quotes thing. I know it technically makes a difference to processing, but are we seriously at the point where we are enforcing that sort of crap where it will in most cases make little to no difference?

I do agree - but didn't realize it wasn't in the guidelines. It most likely is a case of someone having given this comment to some of my PR's and I've just assumed it was a style guideline. So just ignore. Sorry mate!

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 266 at r2 (raw file):
Previously, BrianFarnhill (Brian Farnhill) wrote…
What method are you meaning? A quick search and I found this - https://github.com/PowerShell/PSDscResources/blob/15b40ddabdf6a46e40fc17d0d07535cf12580c89/DscResources/MSFT_WindowsFeature/MSFT_WindowsFeature.psm1#L549 - but it's doing literally what we are doing, it's just not in a common helper thing in this case. So is the issue here that this should go in the helpers module but the approach is fine? I just wanted to check before processing this change

Nope - please disregard this. It came from a feature branch I never got committed. I apologize for the mistake.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
OK I know about a lot of cmdlets not respecting ErrorAction and have created these kind of solutions for other work as well.

Ok that is useful to know. I hadn't encountered this problem before. Might be worth a comment explaining so I don't forget and decide it makes no sense again :grin:

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Thanks for adding this! Could you remove the 3 trailing spaces after 'NA' please

I'm thinking that once the standard VS Code settings are added to the repo, whitespace automatically gets trimmed - which will be really handy.

Comments from Reviewable

Awesome work @BrianFarnhill! couple of last things and it's good to go. I've added a comment about how to fix the Get-DscResource failure as well.

Reviewed 5 of 7 files at r4.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):
Previously, BrianFarnhill (Brian Farnhill) wrote…
It is used in the code, but the only reason it exists as a cmdlet is so we can mock it. I can mock a cmdlet but I can't (easily) override a system type to mock it's value, so we drop it in this cmdlet for that reason. Happy to update the synopsis to make that clearer if you think it needs it.

The New-Guid function that ships with PSv5+ does a simular thing. Looking at it's synopsis: Creates a GUID
Looking at it's definition:
        [OutputType([System.Guid])]
    Param()

    Begin
    {
        [Guid]::NewGuid()
    }

Why I point this out.. The function is used in the code as a normal function. IMO it should just state what it does and not that for you the purpose was mocking :smile: Make sense?
Synopsis would be, Returns the OS version.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):

function Get-OSVersion
{
    return [Environment]::OSVersion.Version

there is no need for the return statement here. Please remove it

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r4 (raw file):

        Name                  = $switch.Name
        Type                  = $switch.SwitchType
        NetAdapterName        = $netAdapterName

It was mentioned in issue 95 that Get-DscResource is now broken with the updates from this PR.
The schema of the resource is updated so that NetAdapterName is a array of strings [string[]]
What you need to do here to fix the issue is typecast the netAdapterName variable as an array of strings.
$returnValue = @{
    Name                  = $switch.Name
    Type                  = $switch.SwitchType
    NetAdapterName        = [string[]]$netAdapterName
    AllowManagementOS     = $switch.AllowManagementOS
    EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
    Ensure                = $ensure
    Id                    = $switch.Id
    NetAdapterInterfaceDescription = $description
}

Get-DscResource will now return properly.
An integration test would have pointed out the issue when testing locally. If you feel like it, you could contribute a basic integration test which shouldn't run on appveyor (I'm not making that a requirement for this PR though so if don't want to, please raise an issue for it instead).

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 6 at r4 (raw file):

    [Key, Description("Name of the VM Switch")] String Name;
    [Key, Description("Type of switch"), ValueMap{"External","Internal","Private"}, Values{"External","Internal","Private"}] String Type;
    [Write, Description("Network adapter name/s for external switch type")] String NetAdapterName[];

please update to name(s) instead of name/s

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 27 at r4 (raw file):

        # Function to create a exception object for testing output exceptions
        function Get-InvalidArguementError

Isn't it Argument?

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):

            return $errorRecord
        } # end function Get-InvalidArguementError

please remove the spaces from the newline

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 109 at r4 (raw file):

        }

While you are at it, could you remove excessive newlines here please?

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 170 at r4 (raw file):

        }

Please remove excessive new lines

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 175 at r4 (raw file):

        }

Too many newlines here as well

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 291 at r4 (raw file):

            # Test Test-TargetResource when the version of Windows doesn't support BandwidthReservationMode
            It 'Invalid Operating System Exception' {

Please remove spaces on newline. Great you added this test!!

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 312 at r4 (raw file):

            }
        }

Please remove these excessive newlines as well

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 371 at r4 (raw file):

                                -ErrorId 'BandwidthReservationModeError' `
                                -ErrorMessage $LocalizedData.BandwidthReservationModeError
                {Set-TargetResource -Name 'WeightBRM' -Type 'External' -NetAdapterName 'SomeNIC' -AllowManagementOS $true -BandwidthReservationMode 'Weight' -Ensure 'Present'} | Should Throw $errorRecord

Great! Could you also add a test where the OS version is correct. Test it to should not throw

Comments from Reviewable

Review status: all files reviewed at latest revision, 21 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 313 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Ok that is useful to know. I hadn't encountered this problem before. Might be worth a comment explaining so I don't forget and decide it makes no sense again :grin:

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 404 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
The New-Guid function that ships with PSv5+ does a simular thing. Looking at it's synopsis: Creates a GUID
Looking at it's definition:
        [OutputType([System.Guid])]
    Param()

    Begin
    {
        [Guid]::NewGuid()
    }

Why I point this out.. The function is used in the code as a normal function. IMO it should just state what it does and not that for you the purpose was mocking :smile: Make sense?
Synopsis would be, Returns the OS version.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
there is no need for the return statement here. Please remove it

What's wrong with specifically stating it rather than just assuming it hits the output stream and comes out that way?

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 23 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
There are 4 spaces here, could you remove them please so it's just a blank line

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Could you add Synopsis and Parameter help for all functions
See functions-have-comment-based-help for guidance

Can I challenge you on this one? Since there is no way to call the help for these directly from DSC why are we including them here? I know in other modules we don't (we do include it for cmdlets and functions that aren't actually DSC ones, so things that someone might legitimately call get-help on) so I'm curious why we would do it like this here?

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 71 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Could you reduce the amount of blank newlines to just 1 and make sure the newline isn't filled with spaces.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 76 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
It was mentioned in issue 95 that Get-DscResource is now broken with the updates from this PR.
The schema of the resource is updated so that NetAdapterName is a array of strings [string[]]
What you need to do here to fix the issue is typecast the netAdapterName variable as an array of strings.
$returnValue = @{
    Name                  = $switch.Name
    Type                  = $switch.SwitchType
    NetAdapterName        = [string[]]$netAdapterName
    AllowManagementOS     = $switch.AllowManagementOS
    EnableEmbeddedTeaming = $switch.EmbeddedTeamingEnabled
    Ensure                = $ensure
    Id                    = $switch.Id
    NetAdapterInterfaceDescription = $description
}

Get-DscResource will now return properly.
An integration test would have pointed out the issue when testing locally. If you feel like it, you could contribute a basic integration test which shouldn't run on appveyor (I'm not making that a requirement for this PR though so if don't want to, please raise an issue for it instead).

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 90 at r4 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I'm thinking that once the standard VS Code settings are added to the repo, whitespace automatically gets trimmed - which will be really handy.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Could you revert these (I see a couple). $null =  is preferred over | Out-Null in general.

According to who? This way we don't end up with an unused variable (which will get highlighted in VS Code and I suspect means will turn up in the PS Script Analyser as an unused variable as well).

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 341 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
could you remove the 1 excessive new line and also the spaces on the other

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 407 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
These messages should also be localized as with the error messages.

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 471 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
please remove trailing spaces

Done.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.schema.mof, line 6 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
please update to name(s) instead of name/s

Done.

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 27 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Isn't it Argument?

I copy/pasted this from the tests in the VHD DVD Drive ... Have fixed

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
please remove the spaces from the newline

OK look - I didn't make changes to this test file, and I'm really starting to feel that I'm being asked to clean up everyone elses code rather than just get the change through that I started in the first place. The only reason this file got updated at all was based on me needing to introduce a change to how the error handling got done in the main module. I get that you all want to push towards meeting the style guidelines and I'm more than happy to do that for code I added, but I'm not going to go through and do that right now for  every file that I touch a handful of lines in - it's just not practical. So I won't be making any further changes to this file.

Comments from Reviewable

Thank you @BrianFarnhill!! You've gone out of your way :smile:  Sorry to be so pushy..
Please address the last couple of points and I'm sure we can merge this asap

Reviewed 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):
Previously, BrianFarnhill (Brian Farnhill) wrote…
What's wrong with specifically stating it rather than just assuming it hits the output stream and comes out that way?

return is a keyword with special meaning / function. In this occurrence it isn't necessary to use. Also it is totally legitimate to assume that the resulting object is send to the output stream.
When compared to the return statements in Test-TargetResource, these serve the purpose of breaking out of processing the rest of the code.
return doesn't add value in this case so please remove it.

---

*[DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4](https://reviewable.io:443/reviews/powershell/xhyper-v/96#-KnY3j1y5SV75i2SqF2j:-KncFYgDRMKjn6iUhqby:b5t49ib) ([raw file](https://github.com/powershell/xhyper-v/blob/8b75e8cd5ea9c06bd2e8b94667ab19d7d163619f/DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1#L24)):*
<details><summary><i>Previously, BrianFarnhill (Brian Farnhill) wrote…</i></summary><blockquote>

Can I challenge you on this one? Since there is no way to call the help for these directly from DSC why are we including them here? I know in other modules we don't (we do include it for cmdlets and functions that aren't actually DSC ones, so things that someone might legitimately call get-help on) so I'm curious why we would do it like this here?
</blockquote></details>

OK I'll do this myself together with the new line / spaces removal left over after this PR is merged (you've done so much to improve this resource already!).

No need to challenge me :smile:, it is stated in the guideline and I'm just **trying** to enforce it. If you don't like the comment based help in here, please raise an issue at the [DscResource GitHub repo](github.com/powershell/dscresources).

---

*[DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4](https://reviewable.io:443/reviews/powershell/xhyper-v/96#-KnY6gxF-35qfKaTd69m:-KncGRNCuenLMNiDjqnr:b-8xz4v0) ([raw file](https://github.com/powershell/xhyper-v/blob/8b75e8cd5ea9c06bd2e8b94667ab19d7d163619f/DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1#L220)):*
<details><summary><i>Previously, BrianFarnhill (Brian Farnhill) wrote…</i></summary><blockquote>

According to who? This way we don't end up with an unused variable (which will get highlighted in VS Code and I suspect means will turn up in the PS Script Analyser as an unused variable as well).
</blockquote></details>

It is common sense that piping something to Out-Null is overhead (slower, more cycles, etc) compared to assigning $null or casting to void. I don't believe there is an official statement from the PG but you can measure this yourself.

I just checked vs code and it doesn't complain about $null being an unassigned variable. PSScriptAnalyzer is used in vs code so it on it's own won't complain about it either.

![2017-06-27\_08-24-12.png](https://files.reviewableusercontent.io/images/reviewable/10071039/WV8ctvQGoQobDkG8vDOipvfs/2017-06-27_08-24-12.png)

Please revert the change

---

*[Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4](https://reviewable.io:443/reviews/powershell/xhyper-v/96#-Kn_sznzRRJouL1jfajC:-KncEAMyPEmIMoWds-d7:bdxylfz) ([raw file](https://github.com/powershell/xhyper-v/blob/8b75e8cd5ea9c06bd2e8b94667ab19d7d163619f/Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1#L50)):*
<details><summary><i>Previously, BrianFarnhill (Brian Farnhill) wrote…</i></summary><blockquote>

OK look - I didn't make changes to this test file, and I'm really starting to feel that I'm being asked to clean up everyone elses code rather than just get the change through that I started in the first place. The only reason this file got updated at all was based on me needing to introduce a change to how the error handling got done in the main module. I get that you all want to push towards meeting the style guidelines and I'm more than happy to do that for code I added, but I'm not going to go through and do that right now for  every file that I touch a handful of lines in - it's just not practical. So I won't be making any further changes to this file.
</blockquote></details>

OK, I understand why this is getting a little frustrating to you and I agree with you it shouldn't be your work to fix every detail in here. My apologies to have pushed you with this (you have fixed a lot already!! thank you). I'll fix the rest myself.

Please put ```Won't fix``` in the other review comments so I can acknowledge them

---

*Comments from [Reviewable](https://reviewable.io:443/reviews/powershell/xhyper-v/96#-:-KncM7XaxxkvmyIUa5ga:b-g9wdai)*
<!-- Sent from Reviewable.io -->

Review status: 6 of 7 files reviewed at latest revision, 11 unresolved discussions.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 408 at r2 (raw file):
Previously, bgelens (Ben Gelens) wrote…
return is a keyword with special meaning / function. In this occurrence it isn't necessary to use. Also it is totally legitimate to assume that the resulting object is send to the output stream.
When compared to the return statements in Test-TargetResource, these serve the purpose of breaking out of processing the rest of the code.
return doesn't add value in this case so please remove it.

Done. I still disagree with you on this point but will just make the change at this point

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 24 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
OK I'll do this myself together with the new line / spaces removal left over after this PR is merged (you've done so much to improve this resource already!).
No need to challenge me :smile:, it is stated in the guideline and I'm just trying to enforce it. If you don't like the comment based help in here, please raise an issue at the DscResource GitHub repo.

Sorry the reason I challenged you is because I believe the intent on the guideline there is that you should have comment based help where it will actually be used by get-help, since there is no way to pull that comment based help out for a DSC resource's get/test/set functions there is no need to have it on those functions. That's the reason that I wrote (and @PlagueHO integrated in to the main DSCResources repo) a bunch of documentation helpers for DSC modules, so we could do help in ways that would actually be surfaced to end users. Anyway if you're happy to let this go through without it on there then it's fine - just wanted to put that thought out there for you.

DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1, line 220 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
It is common sense that piping something to Out-Null is overhead (slower, more cycles, etc) compared to assigning $null or casting to void. I don't believe there is an official statement from the PG but you can measure this yourself.
I just checked vs code and it doesn't complain about $null being an unassigned variable. PSScriptAnalyzer is used in vs code so it on it's own won't complain about it either.

Please revert the change

Sure

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 50 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
OK, I understand why this is getting a little frustrating to you and I agree with you it shouldn't be your work to fix every detail in here. My apologies to have pushed you with this (you have fixed a lot already!! thank you). I'll fix the rest myself.
Please put Won't fix in the other review comments so I can acknowledge them

Won't fix

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 109 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
While you are at it, could you remove excessive newlines here please?

Won't fix

Tests/Unit/MSFT_xVMSwitch_BandwidthReservationMode.Tests.ps1, line 371 at r4 (raw file):
Previously, bgelens (Ben Gelens) wrote…
Great! Could you also add a test where the OS version is correct. Test it to should not throw

Won't fix

Comments from Reviewable

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Thanks @BrianFarnhill!


/W Parse.InterferenceCheck

Using the Dynamo version Revit <IP_ADDRESS>52 and Core <IP_ADDRESS>.85 I got null on all outputs.

Unziping the node I got this

Same on version 2.1.0.XXXX

Around v 2.3 the ironpython engine was updated and changed the way files are read when using the native python file reader. The best solution I've found is to completely avoid it and use the C# System.IO methods.
As a quick fix, you'll need to change the following lines:

with System.IO.File.OpenText(report1) as f:
	while not f.EndOfStream:
		parser.feed(f.ReadLine().replace("&", ""))

I'll be sure to include these fixes in the next release


xds-k8s jobs: standardize TESTING_VERSION

This Addresses the issue with skips not working in golang tests, ref b/235688697.

Unifies TESTING_VERSION detection  in grpc_xds_k8s_install_test_driver.sh - new approach applicable to all languages.
Use TESTING_VERSION in all build files in --testing_version and when tagging docker images. This will be backported to all active test branches. Build Scripts in all other languages will be updated as well.

grpc/core/master/linux/psm-security
grpc/core/master/linux/grpc_xds_k8s_lb
grpc/core/master/linux/grpc_xds_url_map


Proposal: ionic start in current working directory

Right now, ionic start . is interpreted as "create a project named .".
$ mkdir project; cd project
$ ionic start .
Please name your Ionic project something meaningful other than '.'

Instead, ionic start . could take the project name from the current working directory
$ mkdir project; cd project
$ ionic start .
Creating Ionic app in folder /home/me/project

I don't think this behaviour is surprising, but the user could be asked to approve if ionic is running in an interactive shell (ionic start is not a command that you repeatedly run):
$ ionic start .
Create project in current directory? [Y/n]

Greetings @slezica!
My sensors indicate that you need to update your issue through our custom issue form. We are now requiring all issues to be submitted this way, to ensure that we have all of the information necessary to fix them as quickly as possible.
Click Here To Update Your Issue
I will have no choice but to close this issue if it is not resubmitted through the form. Please fill out the rest of the form, so that I may use my friendly robot powers to assist you.
Thank you!

Turning an existing directory in to an ionic project opens a bigger can of worms than the benefits weigh. Sorry, not this time around.

What kind of worms, exactly?

I think the README should probably be changed then, to say, start with ionic start proj tabs then cd into that directory, and then do git add remote, git push.
I think the problem is people make the repo, check it out (or just git init) and then try to run the ionic start command, and it fails, or it makes another folder in their project, and they are confused.


Fix sphinx build

Proposed change

Fixes docs build with sphinx==5.0.0.
Type of change

[x] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (which adds functionality to this library)
[ ] Breaking change (fix/feature causing existing functionality to break)
[ ] Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:

Checklist

[x] The code change is tested and works locally.
[ ] Tests have been added to verify that the new code works.

Codecov Report

Merging #451 (178ab40) into master (f1b6557) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #451   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files           16        16
  Lines         1255      1255
=========================================
  Hits          1255      1255

Flag
Coverage Δ

3.10.x
100.00% <ø> (ø)

3.6
100.00% <ø> (ø)

3.7
?

3.8
100.00% <ø> (ø)

3.9
?

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1b6557...178ab40. Read the comment docs.


More named exports

This adds more named exports.  See #7779.

Looks like a few tests to fix up...

I see a bad import in the overview map.  Will fix it up tomorrow.

I'm taking on the overview map import.


Removed obsolete passphrases

Removed obsolete Apple passphrases

I guess correct way to fix this would be by reading the values from the environment variables.