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

Add support for custom NuGet symbol feed during push #1549

Merged
merged 1 commit into from
May 10, 2017
Merged

Add support for custom NuGet symbol feed during push #1549

merged 1 commit into from
May 10, 2017

Conversation

zvirja
Copy link
Contributor

@zvirja zvirja commented May 8, 2017

Starting from NuGet 3.5 it's now possible to publish package together with its symbols for the custom feeds as well (previously it was possible for the official feed only).

I've extended the NuGetParams with the SymbolPublishUrl and SymbolAccessKey properties, so that NuGetPublish and NuGet functions will publish package with its symbols if the SymbolPublishUrl is specified.

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Looks good.


// Newer NuGet requires source to be always specified, so if PublishUrl is empty,
// ignore symbol source - the produced source is broken anyway.
let normalize str = if str = "" then null else str
Copy link
Member

Choose a reason for hiding this comment

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

If we use option here its probably more idiomatic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please show a sample of your idea? I'm not sure I got it entirely..

Copy link
Member

Choose a reason for hiding this comment

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

I thought like if IsNullOrEmpty str then None else Some str with appropriate changes in the match. It was just an idea, maybe it just looks more ugly in that case ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthid Indeed, with options it looks much better. I've updated my commit.

@@ -91,6 +93,8 @@ let NuGetDefaults() =
WorkingDir = "./NuGet"
PublishUrl = null
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should change this default or do it like nuget and throw an error? (I know its unrelated to this PR, but it might make sense to fix that as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a separate issue about defaults and I'd prefer to fix it via separate PR.

do it like nuget and throw an error

It's a dangerous change, because previous versions of Nuget worked fine without source specified. If we start to require the source now, it would be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yes you are right :)

Since NuGet 3.5 it's possible to specify a custom symbol feed. That
allows to publish package to a custom feed together with its symbols.
@matthid matthid merged commit 0a08458 into fsprojects:master May 10, 2017
@matthid
Copy link
Member

matthid commented May 10, 2017

I'm really a bit sceptical about adding features to FAKE 4 but thanks :)
I think we should do some announcement somewhere I guess?

@zvirja
Copy link
Contributor Author

zvirja commented May 10, 2017

I'm a bit new to FAKE, so am not aware of its plans. Is there something wrong with adding new features to v4?

think we should do some announcement somewhere I guess?

Again, I don't know a lot about your channels. Wouldn't it be enough to simply put this information to the Release Notes? Do you need some actions from my side? :)

@matthid
Copy link
Member

matthid commented May 10, 2017

Nvm I was mostly speaking to myself. Problem is I did a major port to netcore and every feature we add to fake4 I basically need to port again to fake 5. I'm a bit concerned that I cannot keep up with the world when we allow people to still add them... I guess you got lucky and don't need to do anything because I haven't figured it out jet ;)

@zvirja
Copy link
Contributor Author

zvirja commented May 11, 2017

@matthid Got it. That's great that nothing from my side is required :)

Could you please trigger a new release of FAKE 4, so I can consume the changes in the downstream projects?

@zvirja
Copy link
Contributor Author

zvirja commented May 11, 2017

@forki I've noticed that usually you make new releases. So could you please trigger a new FAKE release so I consume the latest changes? 😊

@forki
Copy link
Member

forki commented May 11, 2017

usually I'm also the one qho merges. @matthid we need to talk about process here.

@zvirja
Copy link
Contributor Author

zvirja commented May 11, 2017

@forki @matthid Do you want me to leave you alone here? 😉

@zvirja zvirja deleted the support-nuget-symbols-upload branch May 11, 2017 11:18
@forki
Copy link
Member

forki commented May 11, 2017

released.

@zvirja
Copy link
Contributor Author

zvirja commented May 11, 2017

@forki Thank you 👍

@zvirja
Copy link
Contributor Author

zvirja commented Jun 15, 2017

@matthid BTW, I've noticed that changes I added in PR are left in the obsolete FakeLib. The replacement doesn't contain the feature. Does it mean that I need to fire yet another PR to move this feature to the new location as well?

@matthid
Copy link
Member

matthid commented Jun 15, 2017

@zvirja Nice catch. Seems like this slipped through somehow. Yes a PR would be awesome ;)
thanks in advance and sorry.

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