Skip to content

Refactoring *-PSResourceRepository cmdlets (Register, Get, Unregister) #346

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 95 commits into from
Apr 8, 2021

Conversation

anamnavi
Copy link
Member

@anamnavi anamnavi commented Mar 2, 2021

This PR includes refactoring for Register-PSResourceRepository, Get-PSResourceRepository, and Unregister-PSResourceRepository. Each of these cmdlets performs some CRUD operation on the repository store (APIs for this are in the RepositorySettings class). For the cmdlets that produce output I've created a strongly typed output class (PSRespositoryInfo). I've also added tests for each cmdlet.

Notes:

  • I kept the preexisting method of deserialization for the repository xml file contents. We control the contents of the xml object we deserialize and create them consistently but am open to a more consistent deserialization method congruent with how we will be doing it for the *-PSResource cmdlets.
  • I will create a separate PR for the help docs for these cmdlets.

PR Summary

PR Context

PR Checklist

@anamnavi anamnavi requested review from PaulHigin and alerickson March 2, 2021 21:40
@anamnavi anamnavi self-assigned this Mar 2, 2021
@anamnavi anamnavi changed the title Refactoring WIP: Refactoring Mar 2, 2021
@anamnavi anamnavi requested a review from JamesWTruher March 2, 2021 21:43
Copy link
Member

@alerickson alerickson left a comment

Choose a reason for hiding this comment

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

I left some comments on this but I just wanted to make a meta comment since this pops up a lot throughout the entire repo: kind of a nit, but as we're going through all the files we should change any implicit types to be explicit

@anamnavi anamnavi changed the title WIP: Refactoring *-PSResourceRepository cmdlets (Register) WIP: Refactoring *-PSResourceRepository cmdlets (Register, Get, Unregister) Mar 29, 2021
Update name parameter description to say it now supports wildcards in Name
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

Just a handful of minor changes.

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@anamnavi anamnavi changed the title WIP: Refactoring *-PSResourceRepository cmdlets (Register, Get, Unregister) Refactoring *-PSResourceRepository cmdlets (Register, Get, Unregister) Mar 30, 2021
@PaulHigin
Copy link
Contributor

@anamnavi Can you look at the test failures?

@anamnavi anamnavi removed the request for review from JamesWTruher April 2, 2021 14:59
Copy link
Member

@alerickson alerickson left a comment

Choose a reason for hiding this comment

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

Everything looks good, just left a few nit comments and a couple of general questions

@PaulHigin PaulHigin merged commit a4934fc into PowerShell:development Apr 8, 2021
anamnavi added a commit that referenced this pull request Sep 7, 2022
#346)

* added new RegisterPSResourceRepository.cs with parameters so far

* add PSRepositoryItemInfo

* added passthru, need to complete implementation

* added code and tests for REgister-PSResourceRepository

* add code for Get-PSResourceRepository and move to srcNew

* add files to srcNew

* moved cmdlets and added Set-PSResourceRepository cmdlet

* removed set, get, unregister resource repository cmdlets to put in later PRs

* rename Register-PSResourceRepository cmdlet and references to previous name of cmdlet

* added comemnts where needed

* revert changes accidentally made

* add Get-PSResourceRepository, Unregister-PSResourceRepository so tests for Register can work

* reset changes to settings.json

* revert unintended changes to srcOld folder

* revert unintended changes to testOld

* revert unintended changes to PowerShellGet.psd1 script

* add EOL to some files that were missing it

* remove commented out code from PSGetTestUtils.cs

* changed using statements so that namespace and class name can have same names without error

* fix reference in ArgumentCompleter.cs

* fix some codacy warnings

* make RepositorySettings.cs a static class and update rest of classes that use it

* remove not so helpful comments

* explicitly type variables that were previously implicitly typed as var

* remove unintended writeDebug message

* use yield to return IEnumerable for ArgumentCompleter and remove private helper method

* remove unncessary parameter set names from cmdlets which don't have multiple parameter sets

* store parameter set names as const fields

* remove unused RemotingCapability field from Cmdlets

* RepositorySettings and PSRepositoryItem now belong to UtilClasses namespace and removed older using statements

* replace private fields with Properties, use switch case for parameter set names

* Remove comments

* create helper method to search if repo with name already exists

* change error handling to be non terminating where needed, and continue looping through rest of repositories if error arises

* remove some unneccessary comments

* refactor so there's a helper method calling the Add() API

* call Read() to find existing repo by name in Add()

* make commented description of Add() clearer

* refactor FindRepositoryXML()

* throw error if non implemented parameters are attempted to be used

* refactor CheckRepositoryStore() and how/where it throws errors

* wrap RepositorySettings file handling code with try/catch

* minor fix

* remove unneccessary using statements

* Delete UnregisterPSResourceRepository.Tests.ps1

* include error specific tests for Register-PSResourceRepository

* change Repositories parameter type from List<Hashtable> to Hashtable[]

* add tests for Unregister and refactor RepositorySettings to handle Unregister related errors

* add out errorMsgs to RepositorySettings Read() and refactor GetPSResourceRepository and its tests

* fix ArgumentCompleter issue

* remove uneccesary comments from RepositorySettings

* rename FindRepositoryElement helper method

* refactor FindRepositoryElement code to just return and be more readable

* styling with if keywords

* fix indentation and bracket placement according to PowerShell style standards

* added Debug statements to register

* added try catch to Get and Unregister, changed out string[] errorMsgs syntax

* fix Debug statements and remove unneccessary try catch

* remove shouldSupportProcess from Get

* put check for repo store in BeginProcessing() and change default switch case to use dbg assert

* check if repo name is whitespace

* correct error message typo

* remove unncessary comment

* fix typo in tests, rename errorMsgs to errorList

* make PSRepositoryItem sealed class, add ShouldProcess() support to Register

* add ShouldSupportProcess = true to Unregister

* don't write error if names matching wildcard aren't found

* not set errorList to null as not needed

* add powershell helper to add quotes to ArgumentCompleter to deal with Name with spaces

* tests use  now

* changed Get and Unregister tests to use FullyQualifiedErrorId

* remove setters from PSRepositoryItem

* remove ValueByPipeline tag from Proxy, ProxyCredential and add onto Repositories

* use ErrorId for Unregister tests

* all tests now rely on FullyQualifiedErrorId

* make shouldsupport() target action message more intuitive

* fix target name formatting for ShouldProcess()

* add newline after brackets and #endregion per powershell style guidelines

* add newline in Utils.cs

* trim trailing and leading whitespaces from Name and throw error if Name is just whitespace

* add test for throwing error if Name is whitespace for register

* rename Get and Unregister to remove hype in cmdlet name

* fix codacy warning

* fixed some naming conventions in test to fix codacy warnings

* renamed PSRepositoryItem -> PSRepositoryInfo to match Amber's output class naming

* Update Name parameter description 

Update name parameter description to say it now supports wildcards in Name

* fix styling issues

* change Repository settings fields to private

* change testcases syntax to Pester v4 syntax

* change temp variable name, update PSResourceRepository.xml file comment

* add newline to end of Utils.cs

* organize using directive statements alphabetically

* change error type for checkRepositoryStore error thrown
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