Skip to content

Unification api and tokens #54

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

Merged
merged 47 commits into from
Jun 9, 2025
Merged

Unification api and tokens #54

merged 47 commits into from
Jun 9, 2025

Conversation

NowinskiK
Copy link
Contributor

@NowinskiK NowinskiK commented May 23, 2025

Pull Request

Pull Request (PR) description

This pull request introduces a range of updates across multiple files, focusing on token management improvements, module version updates, and workflow enhancements. Key changes include replacing outdated authentication methods with a streamlined token refresh mechanism, updating dependencies to newer versions, and refining development and testing workflows for better clarity and efficiency.

Token Management Improvements:

  • Replaced Set-FabricAuthToken with Update-FabricToken for token updates and streamlined user context changes using Connect-FabricAccount. (README.md README.mdL69-R74)
  • Removed Confirm-FabricAuthToken and extended Test-TokenExpired to support automatic token refresh using the EnableTokenRefresh feature flag. (CHANGELOG.md [1] source/Private/Test-TokenExpired.ps1 [2] [3] [4] [5]
  • Updated all public functions to replace Confirm-FabricAuthToken calls with Test-TokenExpired for consistent token validation. (source/Public/Capacity/*.ps1 [1] [2] [3] [4] [5] [6] [7]

Dependency Updates:

  • Updated Az.Accounts dependency from version 4.2.0 to 5.0.0 in both RequiredModules.psd1 and source/FabricTools.psd1. (RequiredModules.psd1 [1] source/FabricTools.psd1 [2]
  • Added a HelpInfoURI to source/FabricTools.psd1 for improved documentation access. (source/FabricTools.psd1 source/FabricTools.psd1L134-L140)

Workflow Enhancements:

  • Changed runs-on configuration in .github/workflows/pr.yml to use windows-latest instead of windows-2019 for testing stages. (.github/workflows/pr.yml [1] [2]
  • Revised development instructions in CONTRIBUTING.md to emphasize using clean PowerShell sessions and dependency resolution. (CONTRIBUTING.md [1] [2]

Codebase Simplifications:

  • Removed obsolete functions and merged logic into existing ones, such as replacing Set-FabricApiHeaders with Connect-FabricAccount. (CHANGELOG.md CHANGELOG.mdR45-R47)
  • Simplified token expiration handling by consolidating logic into Test-TokenExpired and removing redundant parameters. (source/Private/Test-TokenExpired.ps1 [1] [2]

Testing Configuration Adjustments:

  • Reduced CodeCoverageThreshold in build.yaml from 0.35 to 0.30 for Pester tests. (build.yaml build.yamlL107-R107)

Fixed

Removed

  • Removed Invoke-FabricAPIRequest and replaced it by Invoke-FabricRestMethodExtended
  • Removed Confirm-FabricAuthToken and extended existing Test-TokenExpired using EnableTokenRefresh Feature Flag
  • Removed Set-FabricApiHeaders and merged the entire logic to Connect-FabricAccount

Task list

  • The PR represents a single logical change. i.e. Cosmetic updates should go in different PRs.
  • Added an entry under the Unreleased section of in the CHANGELOG.md as per format.
  • Local clean build passes without issue or fail tests (build.ps1 -ResolveDependency).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

@NowinskiK NowinskiK linked an issue May 23, 2025 that may be closed by this pull request
Signed-off-by: Rob Sewell <mrrobsewell@outlook.com>
@SQLDBAWithABeard
Copy link
Contributor

Oof a lot to go through here!!

Please can you include in the PR what is being changed to help reviewers to be able to assist :-)

Copy link

github-actions bot commented May 23, 2025

Linux Test Results

4 567 tests   4 566 ✅  43s ⏱️
  405 suites      1 💤
    1 files        0 ❌

Results for commit 3084bd8.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 23, 2025

WinPS51 Test Results

4 567 tests   4 566 ✅  52s ⏱️
  405 suites      1 💤
    1 files        0 ❌

Results for commit 3084bd8.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 23, 2025

WinPS71 Test Results

4 567 tests   4 566 ✅  52s ⏱️
  405 suites      1 💤
    1 files        0 ❌

Results for commit 3084bd8.

♻️ This comment has been updated with latest results.

@SQLDBAWithABeard
Copy link
Contributor

To my eyes it looks ok. Thank you for all the hard work so far.

There are a couple of changes required to make the Pester Tests pass.

You always need a CommandName.Tests.ps1 file in unit tests

You need to pass the ScriptAnalyser rules

You can check these with

Invoke-ScriptAnalyzer -Path .\source\Public\**

You can always run the tests as the PR runs locally with

./build.ps1 -Tasks build,test

We need to get a load of this into the Contributing files to help everyone with these things

Cosmetic changes

Co-authored-by: Jess Pomfret <jpomfret7@gmail.com>
Signed-off-by: Kamil Nowinski <kamil@nowinski.net>
@NowinskiK
Copy link
Contributor Author

Thanks @jpomfret for your time and efforts reviewing this big PR! Much appreciate!
Let's leave general suggestions as separate issues (I created one for splatting).
I agree we need to define standards, but that's the lower priority IMHO.
We should ensure that most of the function works correctly, user user-friendly and are covered by plenty of unit tests.
I hope you get my point. :)

Co-authored-by: Jess Pomfret <jpomfret7@gmail.com>
Signed-off-by: Kamil Nowinski <kamil@nowinski.net>
@jpomfret
Copy link
Contributor

jpomfret commented Jun 6, 2025

@NowinskiK - I've been through all the files and added a few more comments - once these are resolved I'm happy to approve this and get it in.

Thanks for all the hard work on this!

(Going forward it'll be best to try and chunk PRs into smaller more manageable ones - but I know we have some overarching changes that need to touch a lot)

@tiagobalabuch
Copy link
Contributor

@NowinskiK amazing work in this PR.

@NowinskiK
Copy link
Contributor Author

@jpomfret thanks for all your comments, efforts and time. I know it was a big PR and we should avoid such huge changes moving forward.
@tiagobalabuch @SQLDBAWithABeard thanks for your input as well.
With the last commit I've just pushed, I think we have all the important concerns resolved.

@NowinskiK NowinskiK merged commit 0c93cfb into develop Jun 9, 2025
8 checks passed
@jpomfret jpomfret deleted the unification-api-and-tokens branch June 11, 2025 12:08
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.

Standardise API calls New-FabricDataPipeline - issue with call to Invoke-FabricAPIRequest
4 participants