Raise error if pinned hash from .configure file is not found

[mokagio]

I just run into this edge case while attempting to update a repository.

bundle exec run configure_update crashed on me because it tried to compare nil and a number with >=.

The nil value was the result of looking for the index of the pinned commit in .configure in the .mobile-secrets repository.

I believe that occurred because, when prompted

[13:24:11]: The current branch is 2 commit(s) behind. Would you like to update it? (y/n)

I selected no, resulting in my local copy of .mobile-secrets to be two commits out of date.

For the record, I selected n because I interpreted the prompt as a suggestion to update the pinned has in .configure, not the .mobile-secrets repository. The messages to STDOUT from the tool's internal made me think it already pulled the repo.

Regardless of how I ended up in that inconsistent state, I think this early failure and error message would have helped me.

For reference, below is the stacktrace:

(/path/to/repo/vendor/bundle/ruby/2.7.0/bin/fastlane) Traceback (most
recent call last):
[...]
8: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-2.210.0/fastlane/lib/fastlane/runner.rb:229:in
`execute_action'
7: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-2.210.0/fastlane/lib/fastlane/runner.rb:229:in
`chdir'
6: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-2.210.0/fastlane/lib/fastlane/runner.rb:255:in
`block in execute_action'
5: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-2.210.0/fastlane/lib/fastlane/actions/actions_helper.rb:69:in
`execute_action'
4: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-2.210.0/fastlane/lib/fastlane/runner.rb:263:in
`block (2 levels) in execute_action'
3: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-plugin-wpmreleasetoolkit-5.4.0/lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_update_action.rb:20:in
`run'
2: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-plugin-wpmreleasetoolkit-5.4.0/lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_update_action.rb:94:in
`configure_file_is_behind_repo'
1: from
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-plugin-wpmreleasetoolkit-5.4.0/lib/fastlane/plugin/wpmreleasetoolkit/helper/configure_helper.rb:98:in
`configure_file_is_behind_local'
/path/to/repo/vendor/bundle/ruby/2.7.0/gems/fastlane-plugin-wpmreleasetoolkit-5.4.0/lib/fastlane/plugin/wpmreleasetoolkit/helper/configure_helper.rb:109:in
`configure_file_commits_behind_repo': \e[31m[!] undefined method `>=' for
nil:NilClass\e[0m (NoMethodError)

[mokagio]

[AliSoftware]

For the record, I selected n because I interpreted the prompt as a suggestion to update the pinned has in .configure, not the .mobile-secrets repository. The messages to STDOUT from the tool's internal made me think it already pulled the repo.

To be honest, the configure tool has always confused me a lot in terms of its terminology, commands and log messages / prompts. I never remember which of the subcommands do what (update, apply, etc) in which direction (encrypt secrets from mobile-secrets into .enc files in repo? Copy secrets from mobile-secrets raw for immediate local consumption? Decrypt .enc files?), nor what the prompts shown by the tool really mean ("Would you like to update [something unclear about what it will update and in which direction]?".

I'd really love if we could make some improvement in the self-documentation and log messages of this tool at some point, to avoid a lot of confusion around it 😓

[AliSoftware]

Error message seems clear enough to me.

I think we can improve the tool logic that it uses to find the distance between current and requested commit, but I'll let you decide if it's worth working on this in this PR, or a subsequent one.

[AliSoftware]

Not sure why the tool uses this kind of solution instead of using ad-hoc git commands like git -c "#{repository_path}" rev-list --count #{configure_file_commit_hash}..HEAD for example…

[mokagio]

That's cool. I'll implement in the context of this PR because there's no rush to merge it as it.

[AliSoftware]

👍 In that case you might also want to take the occasion to look at the git Ruby gem (that I think we already use in other parts of the release-toolkit and is already one of our dependencies) to implement that call instead and to avoid the shell out 🤔

[jkmassel]

Not sure why the tool uses this kind of solution

Because the author (me) was a noob 😜

[AliSoftware]

That's cool. I'll implement in the context of this PR because there's no rush to merge it as it.

@mokagio FYI I am about to make a new release of the toolkit today to ship some new features I want to use in WPAndroid/WCAndroid.

Since you mentioned you'd implement the suggested change above in this PR rather than a separate one, I won't include this PR it in the upcoming release… except if you would like me to merge it as is (modulo CHANGELOG conflict resolution) now to include it after all, and plan for addressing the change in a future subsequent one? 🤷

[mokagio]

Thanks for the ping. Here's another nice to have but not urgent PR that may take a while to get addressed... 🤔

I missed the boat on 6.1.0, #432, so I might as well keep this in the queue in the hope to get to it sooner rather than later.

[mokagio]

Addressed in 09d3863 with some help from Cursor to write the integration tests

Not impressed with the fact that I asked it to help me write tests to support refactoring and it suggested tests that stubbed out the method calls 😓

[jkmassel]

@mokagio, somehow this is still sitting on my review list?

Figured I'd bump it :)

[mokagio]

@jkmassel Thanks for the ping! I got to addressing the Git implementation suggestion in the end.

But that only made me realize that there are various other git... calls done with `` in the code. I resisted the temptation of addressing them here and added a note in #572 instead.

[AliSoftware]

Gosh this feels a bit dirty nowadays compared to when that PR was first opened 2 years ago, and very tempting to migrate the implementation of all this to (1) use the git gem instead of system shell calls directly for a cleaner implementation and tests (2) and thus lift this limitation as a bonus side effect.

I'd also argue that those specs end up being more integration tests (with them creating a dummy local git repo in a temp dir for each spec example) than unit tests focusing on how it's being tested 🤔 (though to be able to write them as unit tests it'd be simpler to update the implementation first so it'd make it easier to mock the git actions in the spec to begin with …)

---

But at this point and given the age of this PR, better finally land it and postpone that refactor for later (as you mentioned above and in #572) than continue pushing the can down the road and never merging this 😅

[mokagio]

I was thinking back to this and I also realized that this should be implemented with the APIs from the git gem.

I'd also argue that those specs end up being more integration tests

That was intentional. I wanted to update the implementation from "shelling out in one single command that calls Git etc." to "uses git gem APIs for (hopefully) better and neater code." The only way to make sure that my rewrite was a correct refactor was to test the end state instead of how the code interacted with Git.

I think this approach is best in the instance of interacting with the outside world, like we do with Git here.

The only other option I can think of that I would consider as good from the testing point of view is what I would do in Swift where I'd wrap Git in a protocol and provide a test double in the tests. But that's not really a thing in Ruby.

[mokagio]

Just waiting for #627 to land and address the Danger issue...