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

NuGetApiKey is required for nuget based gallery service #463

Open
wants to merge 21 commits into
base: development
Choose a base branch
from

Conversation

heidarj
Copy link

@heidarj heidarj commented Apr 12, 2019

Hey, I would like to submit my attempt at fixing the problem described in issue #143

I removed the ValidateNotNullOrEmpty check from the NugetApiKey parameter and implemented if statements checking for the parameter in Publish-Module and Publish-PSArtifactUtility when constructing parameters, in the same way credentials are added conditionally.

I ran tests on my machine after making the changes and got the following results:

Tests Passed: 348, Failed: 10, Skipped: 53

Most of the failures were related to the dotnet cli tool, I have version 3.0.0 preview installed and imagine that might be conflicting.

With this change the cmdlet fails if no apikey is passed, in the same way as if an invalid apikey is passed, with a 403 status code from the API.

alerickson and others added 4 commits March 13, 2019 11:09
Removed _ValidateNotNullOrEmpty_ from  NugetApiKey and implemented
checking for the parameter in Publish-Module and
Publish-PSArtifactUtility similar to how
credentials are added conditionally.
@msftclas
Copy link

msftclas commented Apr 12, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@edyoung edyoung left a comment

Choose a reason for hiding this comment

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

@heidarj thanks! I think this is a fine approach. Please see my comment, and check that the error message you get when you try to publish to psgallery without an apikey is understandable.

$ArgumentList += '-NonInteractive'
if ($PSBoundParameters.Containskey('NugetApiKey')) {
$ArgumentList += @('--api-key', "`"$NugetApiKey`"")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nuget.exe doesn't have an argument --api-key, stick with -apikey

lwajswaj and others added 6 commits May 1, 2019 06:37
* fix prerelease behavior

* these tests are intermittently failing on appveyor (networking issues?)

* delete unused file

* autoformat; add -ErrorAction to some tests to get better info

* fix skip syntax

* extra debug info

* fix count and add more debug info
* facilitate easier local development

* update readme file
- A few of localization strings for the DSC resource PSModule was revised
  to be more descriptive and fixed typos. One localization string was
  removed as it was not used.
* Reusing Tags caused using the Length property to be used like an array instead of string

* Added EnforceMaximumTagLength switch on Publish-Module to limit tags to 4000 characters

* Renamed EnforceMaximumTagLength to SkipAutomaticTags

* Keep Tags as string array and make new variable for a space separated string
@edyoung
Copy link
Contributor

edyoung commented May 12, 2019

@heidarj I'm pretty sure the argument to nuget.exe I call out above is wrong, but otherwise this should be fine. Let us know if you have time to update?

@heidarj
Copy link
Author

heidarj commented May 21, 2019

@edyoung I implemented the localized NuGetApiKeyIsRequiredForNuGetBasedGalleryService error message for when an API key is not specified so that it now throws the same error as before, if the repo you are pushing to requires an API key. Please let me know if any further changes are needed, otherwise I am good with this as is.

@edyoung
Copy link
Contributor

edyoung commented May 21, 2019

Thanks! I think it's still a problem to pass --api-key when calling nuget, because it doesn't support the -- based parameter form. Also looks like the branch needs to be rebased or merged to match what's currently in development. If it's possible to fix that will happily merge.

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.

None yet

8 participants