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

Move targetedOSVersion to targetedOSVersionRule - second attempt at version rules #225

Merged
merged 4 commits into from
Aug 23, 2021

Conversation

erikng
Copy link
Member

@erikng erikng commented Aug 23, 2021

fully addresses: #157

  • targetedOSVersionRule is a simple string with targetedOSVersion fully removed
  • If nudge finds a full match (device running 11.5.2, rule is 11.5.2), it will use the full match rules
  • if nudge finds a partial match (device running 11.5.2, rule is 11), it will use the partial match
  • if neither are found and there is a dictionary missing the targetedOSVersionsRule key, or the key has a value of "default", it will use them. This allows forward/backward compatibility with v1.0

Example JSON:

{
  "osVersionRequirements": [
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.2"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.3",
      "targetedOSVersionsRule": "11.5.2"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.4",
      "targetedOSVersionsRule": "11"
    }
  ]
}

or you could do

{
  "osVersionRequirements": [
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.2",
      "targetedOSVersionsRule": "default"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.3",
      "targetedOSVersionsRule": "11.5.2"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.4",
      "targetedOSVersionsRule": "11"
    }
  ]
}

The main benefit of this approach, is we do not have to repeat any of the key structure compared to #224.

I also anticipated that we would have to clone all keys from OSVersionRequirements as people may want custom aboutUpdateURL or aboutUpdateURLs and this would be a constantly moving target

- targetedOSVersionRule is a simple string with targetedOSVersion fully removed
- If nudge finds a full match (device running 11.5.2, rule is 11.5.2), it will use the full match rules
- if nudge finds a partial match (device running 11.5.2, rule is 11), it will use the partial match
- if neither are found and there is a dictionary missing the targetedOSVersionsRule key, or the key has a value of "default", it will use them. This allows forward/backward compatibility with v1.0
@erikng
Copy link
Member Author

erikng commented Aug 23, 2021

If someone needed forward and backward compatibility they could do something like

During the transition, order would matter, but only for v1.0.0 clients. You would want your "default" rule at the first item of the array.

{
  "osVersionRequirements": [
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.2",
      "targetedOSVersions": [
        "11.0",
        "11.0.1",
        "11.1",
        "11.2",
        "11.2.1",
        "11.2.2",
        "11.2.3",
        "11.3",
        "11.3.1",
        "11.4",
        "11.5",
        "11.5.1"
      ],
      "targetedOSVersionsRule": "default"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.3",
      "targetedOSVersionsRule": "11.5.2"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.4",
      "targetedOSVersionsRule": "11"
    }
  ]
}

@erikng erikng requested a review from natewalck August 23, 2021 14:00
Nudge/Utilities/Utils.swift Show resolved Hide resolved
Nudge/Utilities/Preferences.swift Show resolved Hide resolved
Nudge/Utilities/Preferences.swift Show resolved Hide resolved
Nudge/Utilities/Preferences.swift Show resolved Hide resolved
@erikng
Copy link
Member Author

erikng commented Aug 23, 2021

@natewalck brought up a good point

{
  "osVersionRequirements": [
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.2",
      "targetedOSVersions": [
        "11.0",
        "11.0.1",
        "11.1",
        "11.2",
        "11.2.1",
        "11.2.2",
        "11.2.3",
        "11.3",
        "11.3.1",
        "11.4",
        "11.5",
        "11.5.1"
      ],
      "targetedOSVersionsRule": "default"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.3",
      "targetedOSVersionsRule": "11"
    },
    {
      "aboutUpdateURL": "https://apple.com",
      "requiredInstallationDate": "2021-07-30T00:00:00Z",
      "requiredMinimumOSVersion": "11.5.4",
      "targetedOSVersionsRule": "11"
    }
  ]
}

If you look at the last two items "targetedOSVersionsRule": "11" is duplicated. As coded, the last item would be triggered only. This is an admin issue and I think some documentation notes around this will be sufficient.

If you accidentally pass multiple arrays with duplicated targetedOSVersionRule values, the last array with the duplicated rule will take precedence.

@@ -53,6 +53,9 @@ func getOptionalFeaturesJSON() -> OptionalFeatures? {
// Mutate the profile into our required construct and then compare currentOS against targetedOSVersions
// Even if profile/JSON is installed, return nil if in demo-mode
func getOSVersionRequirementsProfile() -> OSVersionRequirement? {
var fullMatch = OSVersionRequirement()
Copy link
Collaborator

Choose a reason for hiding this comment

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

small nit: you could create an enum with these three cases instead of having three variables. That would result in code that's simpler to read, with a switch case instead of multiple conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I fully understand how to do this, but would like to know more.

I don't know what the admin is passing as it could be 0-> infinite arrays and the keys are optional. If I could get a code example, would help me out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can try to submit a PR after you merge the final version of this change in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good

@erikng
Copy link
Member Author

erikng commented Aug 23, 2021

Going to merge this. @groob said he will try and turn it into an enum when he has time.

@erikng erikng merged commit b162b29 into development Aug 23, 2021
@erikng erikng deleted the version-rules-2 branch August 23, 2021 16:04
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.

None yet

3 participants