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

BREAKING CHANGE: Identity and Resource Refactor and Update #36

Draft
wants to merge 356 commits into
base: main
Choose a base branch
from

Conversation

ZanattaMichael
Copy link

@ZanattaMichael ZanattaMichael commented Jun 5, 2024

Pull Request (PR) description

The following pull request performs a major rework of the identity and resources of the module.

  1. Re-Working the caching mechanism to focus on enumeration and caching at the execution of the script.
  2. Removal of PAT Tokens from Resources.
  3. Removal of Api URL's from Resources.
  4. Rework of authentication and separating it into it's own command: New-AzDoAuthenticationProvider
  5. Introduction of Token retrieval protections to prevent accidental token pipeline leakage.
  6. Added the following resources:
    1. xAzDevOpsProjectGroup
    2. xAzDevOpsOrganizationGroup
    3. xAzDevOpsGroupMember
    4. xAzDoGroupPermission
    5. xAzDoProjectServices
    6. xAzDoGitRepository
    7. xAzDoGitPermission
  7. Renamed resource: AzDevOpsProject to xAzDevOpsProject

Task list

This Pull Request is currently in draft form and will be updated as more resources and features are added.

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@ZanattaMichael ZanattaMichael marked this pull request as ready for review September 25, 2024 09:28
@ZanattaMichael ZanattaMichael marked this pull request as draft September 25, 2024 09:47
@johlju
Copy link
Member

johlju commented Sep 25, 2024

I noticed that the resources have ’x’ prefix, we can remove that as it is no longer used. We are actively trying to remove the ’x’ from module names and resources throughout the DSC Community. But it can be done in another PR too.

Impressive work on this one PR 😊

@johlju
Copy link
Member

johlju commented Sep 29, 2024

@ZanattaMichael was this meant to move out of draft or are you still working on this?

@ZanattaMichael
Copy link
Author

ZanattaMichael commented Sep 29, 2024 via email

@kilasuit
Copy link
Contributor

Ideally I wanna break this into smaller PR's as not to have PR Review Fatigue on this codebase (which I also didn't write so there is compounded fatigue at play here)

@ZanattaMichael what TimeZone you in? I'm UTC +1 atm but around all sorts of hours due to insomnia. Would be good if possible to schedule some pair programming time around this to spilt it and then get this over the line & see if there is a way that we can pack this as a non-breaking change if at all possible.

If we aren't using classes in this we should do & we also need to in longer term look at #27 & pull out the work I identified for #33 that I never got round to doing in 2022 & thanks to @SphenicPaul for the initial commits. as I suddenly lost my dad to cancer & this was meant to be a repo for presenting about at PSConfEU that year, which went out the window.

@ZanattaMichael
Copy link
Author

ZanattaMichael commented Sep 30, 2024

I'm sorry to hear for your loss.
I'm happy to catchup and we can look at breaking this down into more manageable chunks. I'm UTC +10 so we fit in the part of the world where it's you're evening and my morning and vise versa:

https://www.timeanddate.com/worldclock/meetingtime.html?iso=20240930&p1=47&p2=136

I'm not sure that can be done since the authentication mechanism in this module was coupled directly to the resource meaning you would need to specify the PAT token for each resource call. This change abstracts the change outside of the module to be called prior to calling the LCM using New-AzDoAuthenticationProvider
The reasoning for this is to add more modern authentication mechanisms like certificate, client secret and managed identity.

Another idea for this module is to approach it like how @nohwnd moved between pester 4.0 and 5.0. From a branch perspective, we could cut a preview version branch that has the new changes and then release a preview.

@kilasuit
Copy link
Contributor

kilasuit commented Oct 1, 2024

Is morning your time best for you? Any particular day suit at all?
I initially planned for all the auth mechanisms to be included, including these ones your adding support for as a Resource property. Will understand better once have dug in and understood the current and potential future states & the development path a bit better.
I think we should be able to do this with no need to break existing resources but perhaps increase the complexity of each a tad, instead of new resources

We shouldn't need to intro it a a breaking change unless there is a change in the underlying API's we'd be using that merits it or we have to drop support for a particular version of the hosted version. Also the less overhead here the better for the management, as long as we keep in line with other DSC Community Modules & the processes used across them all, which is yet another thing that I need to brush up on

@ZanattaMichael
Copy link
Author

ZanattaMichael commented Oct 1, 2024

Mornings are better for me. Let's put this on hold, the coming weeks are going to be chaotic with gardening, and my daughters birthday party. Ideally I wanted the authentication mechanism to be similar to Terraform's model with a Cert, however DSC doesn't support other resources types.

The version that I was working on for work was going to use a custom LCM which uses Invoke-DSCResource. The configuration is based on Datum and borrows aspects from ansible playbooks.

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