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

Rewrite drone-nuget to use drone-plugin-lib #14

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

donny-dont
Copy link

Completely redo the drone-nuget plugin to remove its usage of node.

@donny-dont donny-dont requested a review from tboerger June 6, 2020 17:38
@donny-dont
Copy link
Author

@tboerger I feel like the plugin bits are ready for a review. I was able to successfully push to the public NuGet repository.

My only open question is what to do about arm targets. I feel like they should just be ignored. You can see the tags for https://hub.docker.com/_/microsoft-dotnet-core-sdk/ they do have arm but they're not alpine images.

Copy link
Contributor

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

How do you want to install dotnet on alpine?

plugin/impl.go Outdated Show resolved Hide resolved

Drone plugin to publish files and artifacts to NuGet repository. For the usage information and a listing of the available options please take a look at [the docs](DOCS.md).
Drone plugin to publish files and artifacts to a NuGet repository. For the usage information and a listing of the available options please take a look at [the docs](DOCS.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

It's linking to a deleted files

plugin/impl.go Outdated Show resolved Hide resolved
return fmt.Errorf("file %s isn't a nuspec or a nupkg", file)
}

if !fileExists(file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike this fileExists function

Copy link
Author

Choose a reason for hiding this comment

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

Is there one you do like?

return fmt.Errorf("could not parse source url %s: %w", p.settings.Source, err)
}

logrus.WithFields(logrus.Fields{
Copy link
Contributor

Choose a reason for hiding this comment

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

Just log that while uploading?


// pushPackageCmd pushes a package to the nuget repository.
func pushPackageCmd(path, name, key string) *exec.Cmd {
return exec.Command("dotnet", "nuget", "push", path, "--source", name, "--api-key", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Trace will print the api key?

Copy link
Author

Choose a reason for hiding this comment

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

Its my understanding that if its a secret it'll end up being hidden in the log.

Copy link
Author

Choose a reason for hiding this comment

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

With npm I was able to just create a config file that had the credentials. With Nuget the api key is always encrypted if stored in a file so its not possible for me to just replicate that.

I don't know if we would want something like a cmd library in the plugin lib. Most of this stuff was copied from drone-npm. If that's the case then we can add something that'll handle the case where we would want to control what is printed out directly.

@donny-dont
Copy link
Author

With regards to supporting linux I'm thinking we just support amd64 and that's it.

https://hub.docker.com/_/microsoft-dotnet-core-sdk/ there are some arm but they aren't on alpine.

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.

2 participants