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

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

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@ _None_

### New Features

_None_
- Improve failure message when pinned commit cannot be found during `configure_update` [#410]

### Bug Fixes

14 changes: 11 additions & 3 deletions lib/fastlane/plugin/wpmreleasetoolkit/helper/configure_helper.rb
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
require 'English'
require 'fastlane_core/ui/ui'
require 'fileutils'
require 'git'

require_relative '../models/configuration'

@@ -74,8 +75,10 @@ def self.update_configure_file_commit_hash(new_hash)
### Returns the currently checked out branch for the `~/.mobile-secrets` repository.
### NB: Returns nil if the repo is in a detached HEAD state.
def self.repo_branch_name
result = `cd #{repository_path} && git rev-parse --abbrev-ref HEAD`.strip
result == 'HEAD' ? nil : result
git = Git.open(repository_path)
return nil if git.branches.select(&:current).empty?

git.current_branch
end

### Returns the most recent commit hash in the `~/.mobile-secrets` repository.
@@ -102,11 +105,16 @@ def self.configure_file_is_behind_local
end

def self.configure_file_commits_behind_repo
# Get a sily number of revisions to ensure we don't miss any
# Get a large number of revisions to ensure we don't miss any
result = `cd #{repository_path} && git --no-pager log -10000 --pretty=format:"%H" && echo`
Copy link
Contributor

@AliSoftware AliSoftware Sep 20, 2022

Choose a reason for hiding this comment

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

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@AliSoftware AliSoftware Sep 20, 2022

Choose a reason for hiding this comment

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

👍 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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why the tool uses this kind of solution

Because the author (me) was a noob 😜

Copy link
Contributor

@AliSoftware AliSoftware Dec 1, 2022

Choose a reason for hiding this comment

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

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? 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mokagio mokagio Jan 30, 2025

Choose a reason for hiding this comment

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

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 😓

image

image

hashes = result.each_line.map(&:strip).reverse

index_of_configure_hash = hashes.find_index(configure_file_commit_hash)

UI.user_error!("Could not find Git commit #{configure_file_commit_hash} from `.configure` file in local secrets repository. Please verify your local copy is up to date with the remote.") if index_of_configure_hash.nil?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-09-20 at 1 34 30 pm


# No need to check for this to be `nil` because it comes by reading the
# local `.mobile-secrets` repo itself.
index_of_repo_commit_hash = hashes.find_index(repo_commit_hash)

return 0 if index_of_configure_hash >= index_of_repo_commit_hash
40 changes: 40 additions & 0 deletions spec/configure_helper_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,48 @@
# frozen_string_literal: true

require 'spec_helper'
require 'tmpdir'

describe Fastlane::Helper::ConfigureHelper do
let(:test_repo_path) { Dir.mktmpdir }

def run_git_command(command:, repo_path: test_repo_path)
# Notice that this will break if the command contains arguments in quotes,
# e.g. `commit -m "Test commit"` will be split into `['commit', '-m', '"Test', 'commit"']`
# which is not a valid arguments list for Git.
#
# For the context of these tests, we can deal with this limitation.
system('git', '-C', repo_path, *command.split, %I[out err] => File::NULL)
end
Comment on lines +9 to +16
Copy link
Contributor

@AliSoftware AliSoftware Jan 30, 2025

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


before do
allow(described_class).to receive(:repository_path).and_return(test_repo_path)

run_git_command(command: 'init')
run_git_command(command: 'config user.email test@example.com')
run_git_command(command: 'config user.name Test User')
File.write(File.join(test_repo_path, 'test.txt'), 'test content')
run_git_command(command: 'add test.txt')
run_git_command(command: 'commit -m test')
end

after do
FileUtils.remove_entry test_repo_path
end

describe '.repo_branch_name' do
it 'returns the current branch name when on a branch' do
run_git_command(command: 'checkout -b feature-branch')
expect(described_class.repo_branch_name).to eq('feature-branch')
end

it 'returns nil when in detached HEAD state' do
current_sha = `git -C #{test_repo_path} rev-parse HEAD`.strip
run_git_command(command: "checkout #{current_sha}")
expect(described_class.repo_branch_name).to be_nil
end
end

describe '#add_file' do
let(:destination) { 'path/to/destination' }