Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Chrome as default browser #1164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set Chrome as default browser #1164

wants to merge 1 commit into from

Conversation

emtuls
Copy link
Member

@emtuls emtuls commented Nov 13, 2024

This fixes #822

It also fixes an issue in the function VM-Update-Registry-Value in common.vm where a parameter was not being used and other parameter names were not used correctly.

@emtuls emtuls added the 🌀 FLARE-VM A package or feature to be used by FLARE-VM label Nov 13, 2024
@emtuls emtuls requested a review from Ana06 November 13, 2024 22:02
@emtuls emtuls self-assigned this Nov 13, 2024

[Parameter(Mandatory=$true)]
[ValidateNotNullOrEmpty()]
[string] $data
Copy link
Member

@Ana06 Ana06 Nov 15, 2024

Choose a reason for hiding this comment

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

this change breaks the call to this function in VM-Apply-Configurations used to parse our config:
VM-Update-Registry-Value -name $name -path $path -value $value -type $type -data $data. I think it may possible to remove name from this function as it is a description in the config, but we do use data and removing it, breaks the installer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I missed that one. I was looking at packages that used the function and didn't see it used, so I assumed it was just a mistake. :)

Just seems odd to me to use $value for -Name in Set-ItemProperty and not $name, and then use $data / $validatedData for -Value instead of using $value.

I would think that changing the naming convention to what Set-ItemProperty expects (to mean, also updating the places that use this function) might be a better route to take, but we can address that in another PR. :)

SetDefaultBrowser "chrome"

# Do not show the "Open with" popup
VM-Update-Registry-Value -path "HKLM:\SOFTWARE\Policies\Microsoft\Windows\Explorer" -name "NoNewAppAlert" -value 1 -type "DWord"
Copy link
Member

Choose a reason for hiding this comment

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

The VM-Update-Registry-Value was introduce to parse the configuration, validating the data. We do not use it in any other package and I think we do not need it here. I suggest to use Set-ItemProperty directly:
Set-ItemProperty -Path "HKLM:\SOFTWARE\Policies\Microsoft\Windows\Explorer" -Name "NoNewAppAlert" -Value `a -Type $type "DWord" | Out-Null

@emtuls emtuls force-pushed the default-chrome branch 4 times, most recently from f1794fb to dbb7027 Compare November 15, 2024 17:38
Copy link
Member

@Ana06 Ana06 left a comment

Choose a reason for hiding this comment

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

@emtuls I think this PR is not failing in the CI because of the exit code as you told me, but because you are using VM-Update-Registry-Value:

cmdlet VM-Update-Registry-Value at command pipeline position 1
data
  Confirmation (`-y`) is set.
  Respond within 30 seconds or the default selection will be chosen.
ERROR: Cannot process command because of one or more missing mandatory parameters: data.

Please change it by Set-ItemProperty as I suggested in my previous comment.

@emtuls
Copy link
Member Author

emtuls commented Nov 25, 2024

Ah, I thought I made that change... I overlooked that. Thank you!

@emtuls emtuls requested a review from Ana06 December 2, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌀 FLARE-VM A package or feature to be used by FLARE-VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set Chrome as default browser
2 participants