rewrite macos_userdefaults using corefoundation gem #11898

rishichawda · 2021-08-04T11:32:28Z

Signed-off-by: rishichawda rishichawda@users.noreply.github.com

Rewrite of macos_userdefaults resource using the corefoundation gem.

Description

Replace usage of defaults command and use the preference interface from the corefoundation gem we're developing at corefoundation.

NOTE: The tests in the pipeline would currently fail for this PR since I'm developing it against a local version of the corefoundation gem. All updates are being tracked in PRs on the gem's repo. Once the required changes are released, we can update the Gemfile in this branch.

Related Issue

#10307
#10083
https://github.com/chef/desktop-config/issues/214

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

logwolvy · 2021-08-09T12:23:02Z

@rishichawda This POC code is already reviewed and could really help, #11785

rishichawda · 2021-08-09T17:17:49Z

@logwolvy Yeah I had seen that code when the PR was raised and it was incorrect somewhere in the start itself so I didn't refer to it further. I did go through the thread in the PR though and kept those in mind while working on the resource. 😄

logwolvy · 2021-08-10T06:02:15Z

@logwolvy Yeah I had seen that code when the PR was raised and it was incorrect somewhere in the start itself so I didn't refer to it further.

@rishichawda Could you point to that? Let's check if that works and re-use

rishichawda · 2021-08-12T09:45:24Z

Yeah I think it was the method call in load current value at the start. That wouldn't work. Anyways, the resource has already been updated in this branch so it doesn't seem like a good use of time to go back to an old code and fix it when we don't need it anymore.

logwolvy · 2021-08-12T10:18:21Z

Yeah I think it was the method call in load current value at the start. That wouldn't work. Anyways, the resource has already been updated in this branch so it doesn't seem like a good use of time to go back to an old code and fix it when we don't need it anymore.

That POC code was working and already reviewed, so that was a head start anyway. Moving on, I recommend, checking/reusing as much as possible before starting out since we are time bound in development/review efforts.

logwolvy

Also, this would absolutely require kitchen tests

logwolvy · 2021-09-21T11:14:33Z

lib/chef/resource/macos_userdefaults.rb

        end
      end

      action :delete, description: "Delete a key from a domain." do
        # if it's not there there's nothing to remove
-        return unless current_resource


this need not be changed

logwolvy · 2021-09-21T11:26:26Z

lib/chef/resource/macos_userdefaults.rb

        end
      end

-      action_class do


using action_class would make passing new_resource redundant, a good thing here

action_class context is not available to load_current_resource.

not referring to load_current_resource here

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

tas50 · 2021-09-24T20:51:08Z

chef.gemspec

@@ -52,6 +52,7 @@ Gem::Specification.new do |s|
  s.add_dependency "addressable"
  s.add_dependency "syslog-logger", "~> 1.6"
  s.add_dependency "uuidtools", ">= 2.1.5", "< 3.0" # osx_profile resource
+  s.add_dependency "corefoundation", "~> 0.3.4" # corefoundation framework utilities


the comment should say where we're using it more than what it gives us.

# macos_userdefaults

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda

Added 12aa62c to cover the test case for bug in #10083

lamont-granquist · 2021-09-29T21:23:01Z

lib/chef/resource/macos_userdefaults.rb

-        end
+      def set_preference(new_resource)
+        CF::Preferences.set!(new_resource.key, new_resource.value, new_resource.domain, new_resource.user, new_resource.host)
+      end


don't think this method is getting used (might be causing some confusion over the action_class block)

oh yeah I might have overlooked this after I moved the call directly inside the action.. will remove this now.. Thank you!

the :write action was using this function but :delete was using the .set! method from CF directly. Changed the :write to also use set! method in 858cc22.

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

logwolvy · 2021-10-05T06:52:03Z

lib/chef/resource/macos_userdefaults.rb

-          cmd
-        end
+      action_class do
+        require "corefoundation" if RUBY_PLATFORM.match?(/darwin/)


would it work if this was declared at the top?

actions won't find the module then so it needs to go here 🤷🏻‍♂️

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda requested review from a team as code owners August 4, 2021 11:32

rishichawda mentioned this pull request Aug 9, 2021

Move to rspec 3.x and refactor the gem chef/corefoundation#10

Closed

10 tasks

rishichawda force-pushed the api-based-macos-userdefaults branch 2 times, most recently from b7eaebf to b337b55 Compare August 9, 2021 18:37

rishichawda changed the title ~~[WIP] rewrite macos_userdefaults using corefoundation gem~~ rewrite macos_userdefaults using corefoundation gem Aug 9, 2021

rishichawda requested a review from a team September 14, 2021 05:40

logwolvy reviewed Sep 21, 2021

View reviewed changes

rishichawda added 6 commits September 24, 2021 16:44

rewrite macos_userdefaults using corefoundation gem

55949ca

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

fix(macos_userdefaults): silent fail on set call

f1763c7

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

refactor(macos_userdefaults): remove valid key check

9bf47c7

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

deprecate type and sudo properties

010dbc8

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

remove transoform on hash keys

f182b5e

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

update tests

c5bc23e

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch 2 times, most recently from be057ab to 37cccb1 Compare September 24, 2021 20:27

transform values at source with coerce

e242e28

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch 2 times, most recently from 468c998 to 162e387 Compare September 24, 2021 20:43

tas50 reviewed Sep 24, 2021

View reviewed changes

rishichawda added 2 commits September 25, 2021 02:26

add corefoundation gem dependency

2633eea

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

require corefoundation only on darwin

c39731e

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch from d521866 to c39731e Compare September 24, 2021 20:56

add e2e test for array with dict value

12aa62c

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda commented Sep 28, 2021

View reviewed changes

lamont-granquist reviewed Sep 29, 2021

View reviewed changes

lamont-granquist approved these changes Sep 29, 2021

View reviewed changes

make the set call uniform

858cc22

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch from 2a96bf8 to 858cc22 Compare September 30, 2021 09:38

remove unused variable in unit test

a7c2dd6

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch 2 times, most recently from 4d28605 to 6d61daa Compare October 1, 2021 19:45

require CF in action_class

1b811ab

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

rishichawda force-pushed the api-based-macos-userdefaults branch from 6d61daa to 1b811ab Compare October 4, 2021 09:30

logwolvy reviewed Oct 5, 2021

View reviewed changes

rishichawda added 3 commits October 6, 2021 23:03

functional tests

7c783e3

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

add macos runner to github workflow

842defa

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

fix chefstyle errors

f54b993

Signed-off-by: rishichawda <rishichawda@users.noreply.github.com>

tas50 merged commit 1217fad into main Oct 17, 2021

tas50 deleted the api-based-macos-userdefaults branch October 17, 2021 03:23

jweyer8 mentioned this pull request Nov 3, 2022

Resource macos_userdefaults: causing Chef Automate server error #13326

Closed

rewrite macos_userdefaults using corefoundation gem #11898

rewrite macos_userdefaults using corefoundation gem #11898

Uh oh!

Conversation

rishichawda commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Types of changes

Checklist:

Uh oh!

logwolvy commented Aug 9, 2021

Uh oh!

rishichawda commented Aug 9, 2021

Uh oh!

logwolvy commented Aug 10, 2021

Uh oh!

rishichawda commented Aug 12, 2021

Uh oh!

logwolvy commented Aug 12, 2021

Uh oh!

logwolvy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishichawda left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishichawda Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rishichawda commented Aug 4, 2021 •

edited

Loading

rishichawda left a comment •

edited

Loading

rishichawda Sep 30, 2021 •

edited

Loading