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

Windows settings time #109

Open
wants to merge 87 commits into
base: main
Choose a base branch
from

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Nov 5, 2024


Addresses issue #34 and #100

@Gijsreyn Gijsreyn marked this pull request as ready for review November 5, 2024 06:56
@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 5, 2024

@ryfu-msft And this one. There is still an issue that I have opened at PSDesiredStateConfiguration.

# Timezone values are taken from the list of timezones (Get-TimeZone -ListAvailable).Id
# TODO: Track issue 125 on PSDesiredStateConfiguration repository to add a ValidateSet for time zones
[DscProperty(Key)]
# [ValidateSet(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be a validate script; Ref your linked issue - PowerShell/PSDesiredStateConfiguration#125 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that I can think of, is create a enum without the spaces, and add another helper function to translate it. Might be a bit overkill, so I'm curious what the PS team is going to mention.

AliasesToExport = '*'

# DSC resources to export from this module
DscResourcesToExport = @('Time')
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're not going to add it in this PR, once this PR merges I would request that you add a follow up issue to track the addition of adding clocks for different timezones under HKEY_CURRENT_USER\Control Panel\TimeDate\AdditionalClocks\1

image

Copy link
Contributor Author

@Gijsreyn Gijsreyn Nov 5, 2024

Choose a reason for hiding this comment

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

I'll put it on #100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Trenly I have partially started on this one. Do you mind sharing your thoughts on #100 how we are going to set both 1 and 2 in the registry?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would implement it as a single AdditionalClock resource, which has Properties of Ensure, TimeZone, DisplayName.

For Get -

  • Fetch the values in 1
  • Compare against properties
  • If the properties match, set ensure present and return the current state
  • If the properties don't match, fetch the values in 2
  • Compare against properties
  • If the properties match, set ensure present; otherwise, set ensure absent
  • Return the current state

For Test -

  • Run Get
  • Compare Ensure of the current state to Ensure of the desired state
  • Return result of comparison

For Set it's a bit more complicated, I'd imagine you'd first want to determine if either 1 or 2 had a matching DisplayName, if it does, set the values for that clock. If no display names match, check if either timezone matches, if it does, set the values for that clock. If neither are a match, put it in slot 1 if the enable property of the slot is not set. If slot 1 is already enabled, put it in slot 2 if the enable property of the slot is not set. If slot 2 is already enabled, throw an error that the maximum number of additional clocks have been set.


Alternatively, you could just add a parameter for Id and restrict it to being 1 or 2 and have it be mandatory for Set operations but optional for Get and Test. That's honestly probably the better option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the details. One small question, how will users be able to specify both 1 and 2 in a single configuration document?

Copy link
Contributor

Choose a reason for hiding this comment

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

They call the resource twice - once for setting slot one, and a second time for setting slot two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lack the knowledge on this part. Either clock 1 or clock 2 can be turned on, or both. Shouldn't the user be able to say something like: AdditionalClock1Settings and AdditionaClock2Settings?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need; They can confugure like:

    - resource: Microsoft.Windows.Setting.Time/AdditionalClock
      directives:
        description: Set clock 1
        allowPrerelease: true
      settings:
        Slot: 1
        DisplayName: UTC
        Timezone: UTC
        Ensure: Present
    - resource: Microsoft.Windows.Setting.Time/AdditionalClock
      directives:
        description: Set clock 2
        allowPrerelease: true
      settings:
        Slot: 2
        DisplayName: Current time in Redmond
        Timezone: Pacific Standard Time
        Ensure: Present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool. I'll pick it up after this is merged!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback This needs a response from the author. label Dec 4, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Dec 7, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 8, 2024

@denelon I'm adding you to the chain as well because I can remember you making a remark in the past that you were checking with the Windows team on registry keys. When spawning up a fresh machine from DevHome, I noticed something odd that I hadn't seen before.

image

I just learned about the privacy and security settings, but they bite this PR (a bit). For now, I have added an additional check on the relevant registry keys I could capture when SetTimeZoneAutomatically is $true, but I'm curious about your thoughts. Should we test this, and if it is disabled, don't we do anything? Or do we just continue?

@denelon
Copy link
Contributor

denelon commented Dec 10, 2024

That's a great find!

What settings are getting applied here, and what happens if we don't enable the location permissions?

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 11, 2024

Users must manually enter the time if the location permissions aren't set.

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +115 to +117
[DscProperty()]
[nullable[bool]] $SetTimeAutomatically

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about moving things like SetTimeAutomatically to a separate DSC resource called Time.
When I look at the settings page for TimeZone, I only see SetTimeZoneAutomatically and if that is not true, then we have the option to set the TimeZoneId and DaylightSaving. I think this DSC resource should try to replicate what we see in the settings app. I think this is the best way to prevent scope creep for this individual DSC resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's precisely why I did it into one DSC resource. They bite each other. Even if we separate them both, we have to check what options are already enabled in both cases. If we set the Timezone to automatically, we might need to clean up the manual settings and vice versa.

Comment on lines +141 to +143
if ($currentState::SupportsDaylightSavingProperty) {
$currentState.AdjustForDaylightSaving = [TimeZone]::GetDayLightSavingStatus()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This value is not dependent on whether the time zone supports daylight saving or not. This should just be a registry check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't totally get you here. It is checking the registry with the GetDayLightSavingStatus() method?

Comment on lines +44 to +55
function Test-LocationSettingPermission {
# On Windows 11, the HKLM is the location services, whereas HKCU is the Let apps access your location setting
$registryKeys = @('HKLM:\Software\Microsoft\Windows\CurrentVersion\CapabilityAccessManager\ConsentStore\location', 'HKCU:\Software\Microsoft\Windows\CurrentVersion\CapabilityAccessManager\ConsentStore\location', 'HKCU:\Software\Microsoft\Windows\CurrentVersion\CapabilityAccessManager\ConsentStore\location\windows.immersivecontrolpanel*')
foreach ($key in $registryKeys) {
$res = Get-ItemProperty -Path $key -Name 'Value' -ErrorAction SilentlyContinue
if ($res.Value -ne 'Allow') {
# TODO: Or should we throw an error?
return $false
}
}

return $true
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this should be its own separate DSC resource for just Location. I'm sure there are other settings that will depend on this, not just Time/TimeZone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially, as it affects the TimeZone setting, what's your suggestion? A custom module to inherit those functions?

@microsoft-github-policy-service microsoft-github-policy-service bot added Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Dec 20, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity No activity has occurred on this work item for seven days. label Dec 28, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed No-Recent-Activity No activity has occurred on this work item for seven days. Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants