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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d3a5ff
Fix typo in an informative comment
mokagio Sep 20, 2022
16eb6c4
Raise error if pinned hash from `.configure` file is not found
mokagio Sep 20, 2022
19231eb
Add note for why we don't check for a value being `nil`
mokagio Sep 20, 2022
3fff855
Add changelog entry for #410
mokagio Sep 20, 2022
9c3f507
Merge remote-tracking branch 'origin/trunk' into mokagio/fix-configur…
mokagio Jan 30, 2025
952f393
Add integration tests for `configure_helper` `repo_branch_name`
mokagio Jan 30, 2025
09d3863
Refactor `repo_branch_name` implementation to use Git API
mokagio Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ _None_

### New Features

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

### Bug Fixes

Expand Down
7 changes: 6 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/helper/configure_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 { |s| s.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
Expand Down