Common build provider to hide specific build system #268
Conversation
|
@pascalberger can I get your thoughts on this? Thanks |
There was a problem hiding this comment.
Added some suggestions to discuss. Otherwise LGTM as a base implementation for the parameters. Later other AppVeyor specific functionality should also be abstracted. Long term I would also prefer to get rid of build parameters in its current form in favor of using typed context. But we should wait with this until we have a version released as it will introduce breaking changes.
| IBuildInfo Build { get; } | ||
| } | ||
|
|
||
| public class AppVeyorTagInfo : ITagInfo |
There was a problem hiding this comment.
I would suggest to move AppVeyor specific code into a separate file to make it easier maintainable once we support more build servers.
There was a problem hiding this comment.
Agree with you. Will do it
There was a problem hiding this comment.
LGTM
@gep13 If also OK for you I'll squash the commits and merge the PR
|
@vhatsura Can you rebase on current develop or check the |
|
@pascalberger, |
|
@vhatsura Are you sure Allow edits from maintainers is enabled? I get permission denied when I try to write to the branch. Or maybe you have some branch policy enabled? It would be nice to have it rebased before merging, to get rid of the merge commits (you updated the branch with an additional merge commit). While I could squash together the commits, to get rid of them, GitHub will lost track that these changes are made by you and attribute them to me (if this is OK for you I can also do it this way). |
|
@pascalberger, hmm, yes I'm sure that checkbox is checked. Also I checked branch and repo policies. All looks fine. By the way, I rebased my branch onto upstream. |
|
@vhatsura Sorry, there was some config issue on my side. I've now cleanup the branch (you were creating merge commits from upstream The changes have now been merged, thanks for your contribution 👍 For the future GitHub workflow is easier if you make changes on a separate branch in your fork, as now the |
Base implementation for #259