RiotGamesMinions / thor-scmversion

Thor tasks to manage a VERSION file based on SCM tags
MIT License
58 stars 22 forks source link

Yoshiwaan file and tag split options #41

Open yoshiwaan opened 9 years ago

yoshiwaan commented 9 years ago

Added new methods to better accommodate users working with Chef.

The problem for Chef users currently is that the metadata.rb file needs to reference a semantic version as well, and as such users can use the VERSION file. However when it comes to pushing this into github the version file is either A) ignored, meaning the cookbook doesn't work or B) the version file is one below the git tag, as the tag is created before the VERSION file is incremented and there is no chance to add the VERSION file to the commit.

Using the above methods to the workflow for a Chef user would be something like

Note this was tested with git, but untested in p4.

yoshiwaan commented 9 years ago

Please let me know if you would like more done on the PR (such as updating help) before merging, or anything else you'd prefer.

capoferro commented 9 years ago

If you could write some tests so no one breaks this in the future, I'll be happy to merge it.

chilicheech commented 9 years ago

Files should not be committed back to git via a build system. Why do you need the VERSION file to be committed to git if a tag is already being created? You can still have ThorSCMVersion's current functionality of writing the VERSION file to disk and creating a git tag with that version; and then run berks upload as part of your build to upload the cookbook to Chef, which would include the VERSION file.

yoshiwaan commented 9 years ago
  1. Because in the metadata.rb of the cookbook you can reference the version file for the cookbook version, thus it can be a necessary part of the cookbook
  2. There is a mismatch between the tag created in github and the version file, meaning if you ever need to reference an old version of the cookbook from source control it's wrong if you include the version file (i.e. tags don't match actual cookbook version number) or the cookbook doesn't work if you don't include the file due to the above point

The build system is interacting with git by creating a tag so why not include a file that is related to the tagging too? It's consistent and reproducible. Horses for courses.

I'll have a look at an integration test when I get some time.

On 1 June 2015 at 15:25, Thiago Oliveira notifications@github.com wrote:

Files should not be committed back to git via a build system. Why do you need the VERSION file to be committed to git if a tag is already being created? You can still have ThorSCMVersion's current functionality of writing the VERSION file to disk and creating a git tag with that version; and then run berks upload as part of your build to upload the cookbook to Chef, which would include the VERSION file.

— Reply to this email directly or view it on GitHub https://github.com/RiotGamesMinions/thor-scmversion/pull/41#issuecomment-107735101 .

capoferro commented 9 years ago

I should add that I fundamentally disagree with the need for VERSION in source control, but this feature is benign enough that I don't have a problem merging it.

VERSION should be created as part of your build process which then gets shipped to the Chef server as part of the artifact. You should not commit it to source control.

yoshiwaan commented 9 years ago

Forgive my ignorance on this but I'm not having any luck getting the existing rspec tests to run without any alterations, let alone additions.

All of the bump tests except major fail for me with an error similar to below, which is indicating that the from_path function is returning a version higher than the constructed test class in the spec tests. I'm not sure if this is because of the stub command in spec/lib/thor-scmversion/git_version_spec.rb or if from_path in lib/thor-scmversion/scm_version.rb is actually returning the tags which are higher than the specs:

  2) ThorSCMVersion::GitVersion behaves like scmversion #bump! should bump minor
     Failure/Error: subject.bump!(:minor).to_s.should == '1.3.0'
     RuntimeError:
       Version: 1.3.3-beta.3+build.2 is less than or equal to the existing version.
     Shared Example Group: "scmversion" called from ./spec/lib/thor-scmversion/scm_version_spec.rb:101
     # ./lib/thor-scmversion/scm_version.rb:114:in `bump!'
     # ./spec/lib/thor-scmversion/scm_version_spec.rb:26:in `block (4 levels) in <module:ThorSCMVersion>'
     # ./spec/spec_helper.rb:36:in `block (4 levels) in <top (required)>'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr/util/variable_args_block_caller.rb:9:in `call'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr/util/variable_args_block_caller.rb:9:in `call_block'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr.rb:182:in `use_cassette'
     # ./spec/spec_helper.rb:35:in `block (3 levels) in <top (required)>'

Is there some special environment I need to use to run the rspec tests?

I'm also getting this failure, which I'm not quite sure about. In fact I don't understand this test at all. I thought git branch --contains searched for a commit hash not a branch name, and in any case the searched value in the test is a tag not a branch. This test is in spec/lib/thor-scmversion/git_version_spec.rb as well:

ThorSCMVersion::GitVersion should detect if a commit is contained on a given branch
     Failure/Error: expect(GitVersion.contained_in_current_branch?('0.0.1')).to be_true
       expected 0 to respond to `true?`
     # ./spec/lib/thor-scmversion/git_version_spec.rb:10:in `block (2 levels) in <module:ThorSCMVersion>'
     # ./spec/spec_helper.rb:36:in `block (4 levels) in <top (required)>'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr/util/variable_args_block_caller.rb:9:in `call'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr/util/variable_args_block_caller.rb:9:in `call_block'
     # /Library/Ruby/Gems/2.0.0/gems/vcr-2.9.3/lib/vcr.rb:182:in `use_cassette'
     # ./spec/spec_helper.rb:35:in `block (3 levels) in <top (required)>'

Any help would be appreciated. Once I get these working I'll have a look at writing tests for the new functionality

yoshiwaan commented 9 years ago

Looks like I don't need to add anything to the rspec tests anyway as the bump! method is already covered and that's all I use.

yoshiwaan commented 9 years ago

I've added the integration tests and the git tests pass on my system. It's my first time using cucumber so forgive me if the tests are a bit cucumbersome... -_____-

There's a couple of things worth noting:

  1. I had to change the logic a little so that the integration tests worked and so that the operations behave as you'd expect. Now you can run version:bumpfile multiple times then version:tag the latest version, instead of having to version:tag after every version:bumpfile for things to make sense. In the last iteration bumpfile would bump the VERSION file based on the latest git tag, now it uses the VERSION file itself to source the version, which is what version:tag uses as well. In practice I'd expect to actually run version:tag after each version:bumpfile but at least now running version:bumpfile more than once produces predictable output.
  2. Running version:tag now pulls down all the git tags, so it'll complain if a tag already exists. This means that you can happily now swap between running version:bumpfile/version:tag and your next version:bump will bump based on your latest scm version, so everything plays nicely and sensibly.
  3. I had to change all of the cucumber tests that stated to be_true to to be true for it to work on my machine. I think this is a ruby 2+ thing, so let me know how you would like it for the PR as I'm guessing this is based on ruby 1.9. Currently they're still on to be true
  4. I was unable to get p4 tests to work. I downloaded the p4 binary but the tests obviously need anything more. If you could try running the tests on a machine that supports these tests and let me know if there's anything to change that would be appreciated
  5. I added *.swp files to .gitignore

Is there a development branch I should have this PR pointed at?

Happy to make any changes required, even nitpicky stuff. Better to do it right and all that.

capoferro commented 9 years ago

Thanks! I'll take a look at this shortly.

capoferro commented 9 years ago

I'm sorry for the neglect. The permissions on this repo are currently incorrect and I'm unable to merge. I'll track down a fix and get this in or have someone else merge.