Skip to content

Refactor SetPSResourceRepository cmdlet #359

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

Conversation

anamnavi
Copy link
Member

@anamnavi anamnavi commented Apr 13, 2021

This PR includes refactoring for Set-PSResourceRepository cmdlet. This cmdlet updates repository info, so I also had to update code in the Update() method in RepositorySettings.cs. I also included a -PassThru parameter for this cmdlet, and the output is of type PSRepositoryInfo class (which was created in a previous PR).

PR Summary

summary of changes included in this PR:

  • refactored Set-PSResourceRepository.cs
  • refactored Update() API in RepositorySettings.cs
  • created tests in SetPSResourceRepository.Tests.ps1
  • updated help doc

resolves issues:

PR Context

PR Checklist

@anamnavi anamnavi changed the title Refactor setpsresourcerepository Refactor SetPSResourceRepository cmdlet Apr 13, 2021
@anamnavi
Copy link
Member Author

not sure how to implement the Credential parameter to be honest

@alerickson
Copy link
Member

@anamnavi I think the credential param should be completely removed. If we need it later on with the integration of credential persistence we can add it back then, but there's currently no need for it.

@anamnavi
Copy link
Member Author

I copied the ValueFromPipelineByPropertyName tag in [Parameter] for all the parameters. I notice we allowed Name, URL, Repositories, Priority parameters to be passed in as pipeline input...but

  1. does it make sense for all these parameters? I'm honestly not sure how we decide which parameters we should allow to pass in via pipeline input

  2. if we allow Priority to be passed in shouldn't we also allow Trusted to be passed in via the pipeline?

@anamnavi
Copy link
Member Author

For Set: allow by pipeline just Name, and REpositories. For Register, ok as is.

@@ -114,10 +114,11 @@ public static PSRepositoryInfo Add(string repoName, Uri repoURL, int repoPriorit
/// Updates a repository name, URL, priority, or installation policy
/// Returns: void
/// </summary>
public static void Update(string repoName, Uri repoURL, int repoPriority, bool? repoTrusted)
public static PSRepositoryInfo Update(string repoName, Uri repoURL, int repoPriority, bool? repoTrusted)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update comment (// Returns: void)

@@ -132,28 +133,47 @@ public static void Update(string repoName, Uri repoURL, int repoPriority, bool?
// Get root of XDocument (XElement)
Copy link
Contributor

Choose a reason for hiding this comment

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

On line 129, error is not ArgumentException, the argument is valid. Should be ItemNotFoundException error.

@PaulHigin PaulHigin merged commit e8c9d58 into PowerShell:development Jun 7, 2021
anamnavi added a commit that referenced this pull request Sep 7, 2022
* added implemenation and test for Set

* remove unnecessary comments

* add some summary parameter comments

* remove more comments

* remove NameParameterSetHelper()

* add test and code for detecting if name key in repositories param is null

* update help doc for Set

* fix codacy issues

* add Debug statements

* add newline at end of SetPSResourceRepository.cs file

* used parameterset name const variables instead of parameter set strings

* remove Credential parameter

* remove Proxy and ProxyCredential parameters

* remove Credential parameter from help docs as well

* trim whitespace with Trim(), no param needed

* order directives alphabetically

* remove Priority and URL parameters from pipeline
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