Skip to content

Implemented configuration override for #826 #829

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

Merged
merged 5 commits into from
Apr 16, 2016

Conversation

labmonkey42
Copy link
Contributor

I've tried to implement this in an extensible manner such that other configuration options could be overridden with (e.g. /overrideConfig:foo=bar;tagPrefix=Foo.Data) but at this time only tagPrefix is parsed.

Feedback is certainly welcome.

@labmonkey42
Copy link
Contributor Author

I'm not really used to GitHub workflow, so I'm also not sure how to link this with #826

@gep13
Copy link
Member

gep13 commented Apr 11, 2016

@labmonkey42 bu including #826 in your last post, GitHub took care of that mapping for you 👍

@gep13
Copy link
Member

gep13 commented Apr 11, 2016

@labmonkey42 one thing I would say about your PR (note, I haven't had a chance to look at it in depth) would be that it is in your best interests to create a branch to do your work on. In all likelihood anytime you create a PR for any project, there is likely to be some feedback and modifications required. Simply put, this is much easier to do if the initial PR work is done on it's own branch.

@labmonkey42
Copy link
Contributor Author

@gep13 thanks for the guidance. I thought about creating a branch here in this repo but I figured forking it would allow me to submit a PR without clogging your repo with another branch.

@gep13
Copy link
Member

gep13 commented Apr 11, 2016

@labmonkey42 forking is definitely the first part of the process, but a separate branch is also a standard work flow. For example, let's say our master branch is updated with the latest code from another scope of work, which impacts on your work. You would be able to pull in those changes to your forked master branch, and from there rebase your branch, sorting any merge conflicts. After that, pushing to your branch then automagically updates your PR branch. Then, once pulled in, our repo doesn't actually "see" your branch, so it doesn't clog anything up.

@labmonkey42
Copy link
Contributor Author

@gep13 I see, thanks for the tip!

@pascalberger
Copy link
Member

@labmonkey42 FYI here is the suggested way to work in GitHub projects documented: https://guides.github.com/introduction/flow/

@@ -25,6 +25,7 @@ public static void WriteTo(Action<string> writeAction)
eg /output json /showvariable SemVer - will output `1.2.3+beta.4`
/l Path to logfile.
/showconfig Outputs the effective GitVersion config (defaults + custom from GitVersion.yaml) in yaml format
/overrideconfig Overrides GitVersion config values inline (semicolon-separated key value pairs e.g. /overrideconfig:tag-prefix=Foo)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth mentioning that only tag-prefix is supported at the moment?

…onfiguration directive supported by /overrideconfig
@labmonkey42
Copy link
Contributor Author

Does this need additional work before approval? Are we awaiting further feedback? As I'm not all that familiar with GitHub flows I'm just wondering what are the next steps.

@JakeGinnivan JakeGinnivan merged commit 2189b7c into GitTools:master Apr 16, 2016
@JakeGinnivan
Copy link
Contributor

Thanks @labmonkey42

The delay was not caused by you :)

@labmonkey42
Copy link
Contributor Author

Great, thanks for the update :)

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.

4 participants