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 publish profiles #2366

Merged

Conversation

kmadof
Copy link

@kmadof kmadof commented Aug 7, 2019

Description

I added support for publish profile file. I changed the way how we set default values for deployment arguments, because arguments given from command line override values from publish profile. So when publish profile is given we must set command lin only when they explicitly set, and not when they are not given like it was before. When publish profile is not given I set command line arguments like it was before.

If available, link to an existing issue this PR fixes. For example:

@kmadof
Copy link
Author

kmadof commented Aug 7, 2019

The build failure appears to be not related to my changes.

@matthid
Copy link
Member

matthid commented Aug 7, 2019

Thanks for taking care of this! As this is basically a breaking change we need to think about how we want to handle it...We can:

  • remove the breaking change
  • add new stuff and obsolete the old stuff
  • split the package out of this repository and increase the major version
  • something different?

@kmadof
Copy link
Author

kmadof commented Aug 8, 2019

To remove the breaking change I would have to use nullable types instead of option, but this is not best idea in F# I think.
I'm not familiar with spliting the package out of repository, so I would try first the second option. It will take probably few days.

@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Aug 17, 2019
@github-actions
Copy link

github-actions bot commented Nov 30, 2019

There has not been any activity in this pull request for the last 3 months so it will be closed in 14 days if there is no activity.

@github-actions github-actions bot added the stale label Nov 30, 2019
@kmadof
Copy link
Author

kmadof commented Dec 10, 2019

Sorry for such long break. I want to make a new module SqlPackage and put there my changes, leaving old DacPac module without change. @matthid would be better to revert these commits and make change here in this PR, or maybe create new branch and new PR?

@matthid
Copy link
Member

matthid commented Dec 10, 2019

@kmadof Sure just push ahead or create a new PR whatever you prefer.

@kmadof
Copy link
Author

kmadof commented Dec 11, 2019

@matthid can we remove stale label to be sure that PR won't be closed.

@kmadof
Copy link
Author

kmadof commented Dec 11, 2019

@matthid can you help me find out what I did wrong? I really don't understand how my changes impacted DotNet Core Integration tests.

@matthid matthid removed the stale label Dec 11, 2019
@matthid
Copy link
Member

matthid commented Dec 11, 2019

@kmadof They didn't, I guess we need to update paket due to fsprojects/Paket#3743. I'll try to get the release/next branch fixed

@kmadof
Copy link
Author

kmadof commented Dec 15, 2019

@matthid did you have time to fix release/next?

@matthid
Copy link
Member

matthid commented Dec 15, 2019

Yes, only travis is still doing something weird but it is quite tricky to debug as it happens only on travis

@kmadof
Copy link
Author

kmadof commented Dec 17, 2019

@matthid I see that now all checks passed. Cool. Thanks for that. What's next?

@matthid
Copy link
Member

matthid commented Dec 17, 2019

Sorry for the late review, I asked two small questions and I think one small thing is missing. You added a new documentation file (help/markdown/sql-sqlpackage.md) which is awesome, but for someone to find it you should add links somewhere (probably in the menu in help/templates/template.cshtml)

Otherwise, the approach looks good to me.

@matthid matthid removed the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Dec 17, 2019
@matthid matthid merged commit 990cc69 into fsprojects:release/next Dec 17, 2019
4 checks passed
@matthid matthid mentioned this pull request Dec 17, 2019
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