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

[New resource] IntuneVPNConfigurationPolicyIOS #5386

Merged
merged 12 commits into from
Dec 3, 2024

Conversation

dannyKBjj
Copy link
Contributor

Work in progress

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Work in progress
@dannyKBjj
Copy link
Contributor Author

@microsoft-github-policy-service agree

@ykuijs
Copy link
Member

ykuijs commented Nov 13, 2024

Hi @dannyKBjj, thanks for creating this resource and contributing it to the project. Unfortunately there is still some stuff missing. All of our resources require unit tests (which are also completing successfully) and example files, for example:

Also we need an entry of this new resource in the changelog: https://github.com/microsoft/Microsoft365DSC/blob/Dev/CHANGELOG.md

Could you please add these items to the PR?

@dannyKBjj
Copy link
Contributor Author

Ok, no problem. Will get it done

@ykuijs
Copy link
Member

ykuijs commented Nov 22, 2024

@dannyKBjj Can you please make sure this PR is updated as well, just like the other ones? Then we can review and merge it.

@dannyKBjj
Copy link
Contributor Author

Hi yes, sorry. I'm working on a number of modules that we require and that one's been bumped down my priority list a bit! I'll get it up to scratch like the others early next week.

Not currently working...
@ykuijs
Copy link
Member

ykuijs commented Nov 28, 2024

@ricmestre Since my Intune expertise isn't good enough, I would like to check with you: Can this PR be merged or might this also be an overlap with another resource?

@ricmestre
Copy link
Contributor

@dannyKBjj Could you please apply the same formatting/coding suggestions I gave you on the other PRs and add the missing files Yorick mentioned? I'll review once this is done.

@dannyKBjj
Copy link
Contributor Author

@dannyKBjj Could you please apply the same formatting/coding suggestions I gave you on the other PRs and add the missing files Yorick mentioned? I'll review once this is done.

Yes sorry, this has been on my 'to do' list a while. Will try and get it sorted today.

@dannyKBjj
Copy link
Contributor Author

dannyKBjj commented Nov 28, 2024

I've made the requested changes and pushed it, but I'm not sure what to do with targetedMobileApps. Can't see anywhere to set these settings in the Intune Gui. Pester test script works correctly with it, so should be OK, but would like to test properly.

All other aspects behaving as expected so far as I can tell in my tenant. Can anyone advise how to create a policy to test targetedMobileApps in my tenant?

@ykuijs ykuijs changed the title Module iOS VPN [New resource] IntuneVPNConfigurationPolicyIOS Nov 29, 2024
Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

What is the file called "stop" in this PR.

And an entry in the Changelog is not yet added.

stop Outdated Show resolved Hide resolved
@ricmestre
Copy link
Contributor

@ykuijs Once the unit tests finish this can be merged, I already tested the resource successfully on my dev tenant.

CHANGELOG.md Outdated Show resolved Hide resolved
@ykuijs
Copy link
Member

ykuijs commented Nov 29, 2024

We are almost there. Just need to fix the changelog 👍

@ykuijs
Copy link
Member

ykuijs commented Dec 2, 2024

@dannyKBjj Can you please review and correct the last comment about the location of the changelog entry? Then we can merge this PR afterwards.

Sorry about that!
Copy link
Member

@ykuijs ykuijs left a comment

Choose a reason for hiding this comment

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

LGTM

@ykuijs ykuijs merged commit c85448c into microsoft:Dev Dec 3, 2024
2 checks passed
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