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

AzDevOpsProject: Azure DevOps, REST API Wrapper and AzDevOpsProject, DSC resource #7

Merged

Conversation

SphenicPaul
Copy link
Contributor

@SphenicPaul SphenicPaul commented Nov 18, 2020

Pull Request (PR) description

@johlju / @kilasuit - I couldn't see this had moved much in a number of months and...

  • I had some 'lockdown', annual leave on my hands
  • I've an eye on using this module in my workplace to configure the less-changable aspects of Azure DevOps server (via the REST API) in the coming months
  • I wanted a bit more of a general work/use of PowerShell classes, class-based, DSC resources and Pester
  • I wanted some basis/framework/experience for which to create other, API-facing, DSC Resource modules (for other tools, for example)

As a result, I've put the last few weeks into engineering something to start this module off and used the SqlServerDsc module as a comparison of sorts in getting to this point.

I had an existing set of PowerShell functions that I've used to form the basis of wrappers around the Azure DevOps REST API and I've tried to maintain the majority of the 'wrapper' functionality in a limited set of resources to encourage reuse (there's a little more information on module structure linked to from the 'contribution' document\readme).

This PR isn't completely ready to go (although there's possibly a lot in it - I'd like to think subsquent changes PRs would be much smaller), but I thought I'd put it out for review to get some feedback/improvements/suggestions etc.

Outstanding notes/points/actions:

  • Still need to increase tests and code coverage on remaining items. I'd like to get this to 100% from the outset, if possible. Some of the class-based resource testing is a little more convoluted than I'd want it to be.

    • There are some 'using' references that I've had to hardcode to the 'output' directory path location. I was hitting problems where the classes referenced in tests were not being picked up by code coverage unless they were explicitly referencing the output directory files. Not sure what better option there might be for this as the path will change with each version increment as it stands. There might be a better way to setup classes and modules .. although community guidelines seemed to be limited on classes and class-based resources.
  • I've updated the pipeline definition to:

    • Include Integration Tests (currently only from Windows OS, using PowerShell Core, but could be added to others if needed)
      • I run my own, Integration Tests against my own, test, Organization within Azure DevOps - I'm not sure if there is one setup for this within the 'dsccommunity'? - If not, one might have to be setup in order to run these.
    • Skip code coverage on Windows OS, using regular PowerShell - There seemed to be a performance hit relating to a Pester bug (although code coverage is still inplace on all the other OS test jobs and PowerShell Core on Windows).
  • GitVersion is still currently referencing a 'master' branch, but the default in the repository seems to be 'main'. I'm not clear which branch name is wanting to be taken forward? - Either the default branch name needs to be changed or the 'GitVersion.yml' file needs updating to include the 'main' branch.

  • I'm expecting to update $Pat variables/parameters to use a PSCredential, although unclear of most suitable option at present:

    1. Have every Resource take a $Pat and $ApiUri parameter (where $Pat is updated to be a PSCredential)
    2. Define a new Connection class which holds ApiUri and PAT information, and which is passed between functions
    3. Use the existing Connection class (as part of current, Azure DevOps, client libraries)

(Note: I've steered away from depending directly on the client libraries as it stands)

  • Add another resource, likely AzDevOpsTeam (which would relate within a project) to ensure it doesn't throw up any larger refactoring considerations.

  • Wiki help on the resource is not present yet. Looks like the other DSCCommunity modules take this from the schema.mof files (which this module doesn't have/use). Is there an alternative guideline to coding something using reflection (or similar) that does this? ... and perhaps adding parameter description attributes within the DSC resources?

  • Theres possible some tidy up of the testing, helper functions to do that generate/provide test cases etc.

This Pull Request (PR) fixes the following issues

Fixes #3 - Possibly? - This PR and it's changes might make this issue a non-issue

Task list

  • 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. Not applicable? - Class-based resources
  • 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

…meter usage in the 'When called with no/null parameter values/switches' context
…zationName', 'Test-AzDevOpsPat', 'Test-AzDevOpsProjectDescription', 'Test-AzDevOpsProjectId' and Test-AzDevOpsProjectName' functions
…zDevOpsApiRestMethod' functions to add an '.Old' suffix.
…' and 'WaitTimeoutMilliseconds' to be 'Int32' types (rather than UInt32)
…and 'WaitTimeoutMilliseconds' to be 'Int32' types (rather than UInt32). Also implemented use of 'Get-Date' and 'Test-AzDevOpsTimeoutExceeded'.
…ort modules correctly in order to support function, unit tests.
@SphenicPaul
Copy link
Contributor Author

I've merged my most recent changes back into this PR (to update the Enums, DSC Folders and DSC Classes points raised).

Test coverage is currently about 50% (taking into account Unit tests only), largely due the ommission and duplication of class-related code-paths - It seems the functionality/code-paths in the classes are not registering as being tested (likely due to test setup following recent changes) and, in addition, the classes are being duplicated within the code coverage omissions (once in the 'Classes' directory and once within the output '.psm1' file).

I doubt (hope) there is much work to get these tests registering as part of the code coverage (and remove the duplication - I know this can be excluded in the build.yml, but I've left it as it is for now).

I'm pretty sure there is coverage for all bar 5 or so functions/methods, and the examples (which I believe get tested ... up to the point of generating the MOF... as part of the suite of DSC Resource tests as part of the build).

I'll leave this as it is for now until you've had a chance to take a look. I'm unlikely to look at making any changes to this before Sunday (29/11) or Monday, but will pickup/respond to feedback before then if I can.

@kilasuit kilasuit self-assigned this Nov 27, 2020
@johlju johlju requested a review from kilasuit January 24, 2021 09:39
@johlju
Copy link
Member

johlju commented Jan 24, 2021

@kilasuit do you have time to review this one? We need to fix the pipeline too, to support the branch rename. See blog post https://dsccommunity.org/blog/convert-master-to-main/

@SphenicPaul
Copy link
Contributor Author

SphenicPaul commented Jan 24, 2021

FWIW ... I've not been back to this for a little while, but I'm inclined to think that making use of some of the modules suggested by @kilasuit does make sense - There's lots of test coverage in my PR (with the exception of some of the classes) so even if we can get an initial build working, refactoring to use other modules should become much easier/cleaner (and I'm guessing any initial builds don't have to be "published" from day one?).

Meanwhile, given a few items that need doing to get the pipeline up and working, I'd probably suggest an initial (smaller 😅 ) PR to get the build pipeline working off main, but I do still think that it's worth creating a "destructable", Azure DevOps Services collection and linking an API key (or API keys limited to functionality around specific resources) into that instance so the DSC build can perform integration tests against it - I got this working from my own builds and worked quite well (Note: the build template I used is in this PR and includes some modifications from the original one).

I'm happy to create a small PR with just the build-related changes in it if required (i.e. updated template and branch name) but you both may have more access/knowledge to tweak/debug and make any revisions to get the build working but it would be good to get to a point to know if a PR build from another fork/repository works against this repository ... it could also mean that we/I could trickle in smaller changes (possibly items already in this PR but in a set of smaller PRs that are easier to review/update).

Let me know if you need me to do anything - I'm only on Slack intermittently, so tag me here if needed.

@johlju
Copy link
Member

johlju commented Jan 24, 2021

@SphenicPaul We can merge this and build from it. I fixed the pipeline files in the repos so it should build against main branch correctly now. I kicked off the test in this PR to see if it looks good or if there is something easy to fix to get the pipeline to pass (if it does not pass).

@SphenicPaul
Copy link
Contributor Author

@johlju - Looks like the unit tests in the PR were failing due to the lack of code coverage and the integration tests are failing due to a lack of an Integration environment (Azure DevOps Services instance) and related API key.

For now, I've lowered the Unit tests threshold (I intend to put this back up) to try and ensure we can get a build running to completion (even if it's not used initially).... looks like you've done this anyway 😄 .

In order to resolve the integration tests, the following variables need adding to the build/pipeline:

  • AzureDevOps.Integration.ApiUri
  • AzureDevOps.Integration.Pat (set as sensitive variable)

... and...

  • the ApiUri value needs setting to a URI of an Azure DevOps organization (e.g. https://dev.azure.com/someOrganizationName/_apis/) where the organization will be torn down and recreated each time (i.e. don't use this organization for anything you want to keep! 😁) ... there also has to be some consideration to ensure that multiple sets of integration tests can't run similtaniously against the same instance (not sure how that would be handled, initially - not sure if you want to create a new organization for every build? ... I think there is a limit).
  • the ApiKey value needs setting to a key with permissions to effectively do anything within the organization used so the DSC resources can be applied into it.

@SphenicPaul
Copy link
Contributor Author

Also looks like there are now some items/errors relating to missing modules on the Linux boxes and errors with trying to use the Examples which probably need me to make some changes to the scripts.

@johlju
Copy link
Member

johlju commented Jan 24, 2021

I'm pushing some changes to see if we can get the unit tests to work at least. My thinking is that we merge this and then create issues for everything we find that needs fixing. At least this is a baseline to work from.

The problem is that the integration test cannot run for PR's since the secret pipeline values are not added to the build for PR's. That will make the build fail on main branch only, and then its "to late" to fix the build issue for the PR. So I'm thinking we should look at making the integration tests to run manually and each contributor can set there own "destructible" DevOps tenant.

I disabled the integration test in the pipeline for now. I can create an issue for the integration tests part.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 157 files at r1, 7 of 52 files at r2, 4 of 5 files at r4, 1 of 1 files at r5, 3 of 4 files at r7.
Reviewable status: 21 of 137 files reviewed, all discussions resolved (waiting on @kilasuit)

@johlju
Copy link
Member

johlju commented Jan 24, 2021

Unit tests passing now. HQRM tests was running for all OS's an compiling examples does not work xplat yet, moved it to a separate job.

@johlju johlju merged commit 8d43d12 into dsccommunity:main Jan 24, 2021
@johlju
Copy link
Member

johlju commented Jan 24, 2021

Merged. Let's continue the work form here.

@SphenicPaul
Copy link
Contributor Author

OK. Thanks.

Your points make sense - I'll have a think on the Integration tests and post my initial thoughts in the issue (#9).

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.

AzureDevOpsDsc: CI/CD pipeline might need to be modified
4 participants