Skip to content

Refactor and migrate Confirm-UPMConfig into UnitySetup.powershell - #256

Merged
Michael Smith (Exolun) merged 30 commits into
microsoft:developfrom
Exolun:user/mismit/migrate_confirmupm
Jul 10, 2024
Merged

Refactor and migrate Confirm-UPMConfig into UnitySetup.powershell#256
Michael Smith (Exolun) merged 30 commits into
microsoft:developfrom
Exolun:user/mismit/migrate_confirmupm

Conversation

@Exolun

Copy link
Copy Markdown
Contributor

Internally we've had a script called Confirm-UPMConfig for quite some time that provides the utility of scanning for all NPM packaging feeds used by a given Unity project and creating auth tokens for each of them, storing them in the user's .toml file.

This pull request migrates that functionality, to UnitySetup, allowing users to call Update-UPMConfig -ProjectManifestPath to have the script find and update auth tokens for their unity project's NPM for all manifests or a given manifest.

Additionally there are a few structural changes incorporated here as the process is now 3 stages of logic in the code:

  • Import-UPMConfig: Collects all of the scoped registries and entries in the existing .toml file and returns them
  • Sync-UPMConfig: Looks at the registries listed and gets auth tokens for each, refreshing expired existing tokens if found, or adding new entries for new scoped registries.
  • Export-UPMConfig: Writes out the UPM configuration to the .toml file (or creates it if one doesn't exist)

The logic is a bit clunky since Sync does most of the work here. Would appreciate feedback where applicable

Comment thread UnitySetup/UnitySetup.psm1 Outdated

@jwittner Josh Wittner (jwittner) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Definitely a good stride, but we've confused up some of the UPMConfig noun, intermingling it with 'ProjectManifest' in some cases. We want something like this:

Import-UpmConfig | Update-UpmConfig -ProjectManifest <manifestPaths> | Sync-UpmConfig | Export-UpmConfig 

We could support ProjectManifest as a first class citizen too and use that to for loading/reading in the Update-UpmConfig cmdlet.

Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psd1 Outdated
Comment thread UnitySetup/UnitySetup.psd1 Outdated

@jwittner Josh Wittner (jwittner) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lots of code to clean up here, that I understand you are likely not the original author for, but now that we're going OSS with this we need to raise the bar.

Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
Comment thread UnitySetup/UnitySetup.psm1 Outdated
@Exolun

Copy link
Copy Markdown
Contributor Author

This is almost done, but I'll need to move the dynamic install logic to the script leveraging this, and push another commit to fix a runtime error I'm seeing now (dropped a param during a refactor)

- Remove dynamic install of Az.Accounts
- Declare Az.Accounts dependency
+ Rename Sync-UPMConfig to Update-PackageAuthConfig since to potentially expose a public Sync method with just the .toml path as a target.

@jwittner Josh Wittner (jwittner) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a couple renames and I think we're good to go.

Comment thread UnitySetup/UnitySetup.psd1 Outdated
Comment thread UnitySetup/UnitySetup.psd1 Outdated
Comment thread UnitySetup/UnitySetup.psd1 Outdated

@jwittner Josh Wittner (jwittner) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yay, this is a great feature!

@Exolun
Michael Smith (Exolun) merged commit d20b718 into microsoft:develop Jul 10, 2024
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.

3 participants