[ensure_git_status_clean] fix incorrect "ignored" param handling #20976

revolter · 2023-01-07T20:43:50Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Motivation and Context

Fixes #19818.

Description

See also: https://git-scm.com/docs/git-status#Documentation/git-status.txt---ignoredltmodegt

Testing Steps

Replace

gem 'fastlane'

with

gem 'fastlane', git: 'https://github.com/fastlane/fastlane.git', branch: 'fix/unclear-param-documentation'

in your Gemfile.

Run bundle install
Run bundle exec fastlane run ensure_git_status_clean

ObjectiveCesar · 2023-01-17T13:13:08Z

fastlane/lib/fastlane/actions/ensure_git_status_clean.rb

@@ -74,7 +74,7 @@ def self.available_options
                                       type: Boolean),
          FastlaneCore::ConfigItem.new(key: :ignored,
                                       env_name: "FL_ENSURE_GIT_STATUS_CLEAN_IGNORED_FILE",
-                                       description: "The flag whether to ignore file the git status if the repo is dirty",
+                                       description: "The ignored files handling mode if the repo is dirty. The available options are: 'traditional' (default), 'no' and 'matching'",


'traditional' is not the default. Currently the default is 'no'.

myhost:MY_PROJECT user$ git status --porcelain --ignored='traditional' M ***/Info.plist M Frameworks/***/Info.plist M Frameworks/***/ios-arm64/***/Ast M Frameworks/***/ios-arm64/***/Info.plist !! .DS_Store !! ***/Resources/assets/ !! Gemfile.lock !! fastlane/README.md !! fastlane/build/ !! fastlane/report.xml

vs.

myhost: MY_PROJECT user $ git status --porcelain --ignored='no' M ***/Info.plist M Frameworks/***/Info.plist M Frameworks/***/ios-arm64/***/Ast M Frameworks/***/ios-arm64/***/Info.plist

vs.

myhost: MY_PROJECT user $ bundle exec fastlane run ensure_git_status_clean [14:10:46]: ------------------------------------- [14:10:46]: --- Step: ensure_git_status_clean --- [14:10:46]: ------------------------------------- [14:10:46]: $ git status --porcelain [14:10:47]: ▸ M ***/Info.plist [14:10:47]: ▸ M Frameworks/***/Info.plist [14:10:47]: ▸ M Frameworks/***/ios-arm64/***/Ast [14:10:47]: ▸ M Frameworks/***/ios-arm64/***/Info.plist

Oh, I misinterpreted the git documentation. It states that if you only use --ignored, without a value, traditional is used - meaning, it's the same as using --ignored=traditional.

Could you please send me the output of git status --porcelain --ignored='matching' too?

Sure, here you go:

myhost:MY_PROJECT jenkins$ git status --porcelain --ignored='matching' M ***/Info.plist M Frameworks/***/Info.plist M Frameworks/***/ios-arm64/***/Ast M Frameworks/***/ios-arm64/***/Info.plist !! .DS_Store !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***/ !! ***/Resources/assets/***.txt !! ***/Resources/assets/***.xml !! Gemfile.lock !! fastlane/README.md !! fastlane/build/ !! fastlane/report.xml

Oh, I misinterpreted the git documentation. It states that if you only use --ignored, without a value, traditional is used - meaning, it's the same as using --ignored=traditional.

Maybe that differs from git version to git version? It would probably be a good idea to explicitly pass a default value of traditional to git status --porcelain --ignored='' if the argument is not set for the fastlane action.

Looks good!

***:*** jenkins$ bundle exec fastlane run ensure_git_status_clean ignored:'none' [✔] 🚀 [14:10:08]: ------------------------------------- [14:10:08]: --- Step: ensure_git_status_clean --- [14:10:08]: ------------------------------------- [14:10:08]: $ git status --porcelain --ignored='no' [14:10:08]: ▸ M ***/Info.plist [14:10:08]: ▸ M Frameworks/***/Info.plist [14:10:08]: ▸ M Frameworks/***/ios-arm64/***/Ast [14:10:08]: ▸ M Frameworks/***/ios-arm64/***/Info.plist [14:10:08]: ▸ M Gemfile [14:10:08]: ▸ ?? Gemfile.lock [!] Git repository is dirty! Please ensure the repo is in a clean state by committing/stashing/discarding all changes first.

I also tested the other options (traditional and matching) and they work as expected.

2 things you might consider adding:

Trimming the passed value for ignored. If you pass i.e. ignored:'none ' the command will fail with an exception since git status can't handle that.

Checking for emptiness after trimming: If you pass i.e. ignored:'' or ignored:' ' both will fail with an exception.

Both of these are just convenience and not a necessity for me to approve this pr. :)

2 things you might consider adding

Oh, wow, you're amazing! Thanks for catching these too ❤️

It looks like other actions don't strip trailing whitespace, so I'll do the same. Meaning, if you specify 'none ', it will treat it as an invalid value for the ignored parameter, and throw an error (which makes sense IMO).

Also, I think that you shouldn't and can't test this validation. Maybe because FastlaneCore::ConfigItem are already tested, and it doesn't make sense to test them again in the context of each individual action.

You actually did, what I had in mind but was not bold enough to ask for because you did all the work so far. Creating a fixed set of options that can be used as arguments is what I would have done. I am a Swift developer though and it probably would have taken me days to figure this out. 🤣

Great work! Thanks.

because you did all the work so far

It's easier if you already started to dive into stuff like this 😁

Creating a fixed set of options that can be used as arguments is what I would have done.

I'm wondering how come I didn't think of it in the first place 😅

I am a Swift developer though and it probably would have taken me days to figure this out.

Me too. I only rarely wrote Ruby, so I'm still getting the hang of it, which means that I sometimes fix stuff by trial and error 😂

Great work! Thanks.

And great work testing it and catching bugs! And thanks for finding this PR and linking to the issue. I actually did notice the difference between the code and the git man page, but I thought that the = is optional 😂

See also: https://git-scm.com/docs/git-status#Documentation/git-status.txt---ignoredltmodegt

rogerluan

Just my 2c on the docs 🤗

fastlane/lib/fastlane/actions/ensure_git_status_clean.rb

rogerluan

LGTM @revolter 💪 could you just take care of the linter? 😊

revolter · 2023-01-31T18:11:42Z

Sorry, I was in vacation. I fixed it now 😁

rogerluan

🚀

revolter changed the title ~~[ensure_git_status_clean ] Fix unclear param documentation~~ [ensure_git_status_clean ] fix unclear param documentation Jan 8, 2023

ObjectiveCesar mentioned this pull request Jan 17, 2023

ensure_git_status_clean documentation confusing wording for ignored parameter #19818

Closed

ObjectiveCesar suggested changes Jan 17, 2023

View reviewed changes

[ensure_git_status_clean ] fix unclear param documentation

b900a11

See also: https://git-scm.com/docs/git-status#Documentation/git-status.txt---ignoredltmodegt

revolter added this to the Improve a fastlane action milestone Jan 17, 2023

revolter self-assigned this Jan 17, 2023

revolter added 4 commits January 17, 2023 18:45

Fix incorrect test name

ab4b5d6

Remove duplicated test

a8cff5b

Fix misleading local variable name

e8b4a13

Fix typo in test names

99f6ca4

revolter changed the title ~~[ensure_git_status_clean ] fix unclear param documentation~~ [ensure_git_status_clean] fix incorrect "ignored" param handling Jan 17, 2023

revolter force-pushed the fix/unclear-param-documentation branch from a14e3f8 to 92a812b Compare January 17, 2023 16:49

revolter requested review from ObjectiveCesar and rogerluan and removed request for ObjectiveCesar January 17, 2023 16:49

revolter force-pushed the fix/unclear-param-documentation branch from 92a812b to c8aa3c5 Compare January 17, 2023 16:54

[ensure_git_status_clean] fix incorrect "ignored" param handling

1c82399

revolter force-pushed the fix/unclear-param-documentation branch from c8aa3c5 to 1c82399 Compare January 17, 2023 17:00

revolter added 2 commits January 18, 2023 08:35

[ensure_git_status_clean] fix incorrect "no" value handling

7ef34da

[ensure_git_status_clean] add missing invalid values handling

2c0ac27

rogerluan requested changes Jan 20, 2023

View reviewed changes

fastlane/lib/fastlane/actions/ensure_git_status_clean.rb Outdated Show resolved Hide resolved

[ensure_git_status_clean] improve param documentation

07b9694

rogerluan approved these changes Jan 23, 2023

View reviewed changes

Fix RuboCop line length error

8dcf22f

rogerluan approved these changes Feb 9, 2023

View reviewed changes

revolter merged commit e55138d into master Mar 9, 2023

revolter deleted the fix/unclear-param-documentation branch March 9, 2023 20:04

joshdholtz mentioned this pull request Apr 16, 2023

Version bump to 2.212.2 #21203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ensure_git_status_clean] fix incorrect "ignored" param handling #20976

[ensure_git_status_clean] fix incorrect "ignored" param handling #20976

revolter commented Jan 7, 2023 •

edited

ObjectiveCesar Jan 17, 2023 •

edited

revolter Jan 17, 2023

revolter Jan 17, 2023

ObjectiveCesar Jan 17, 2023

ObjectiveCesar Jan 17, 2023 •

edited

ObjectiveCesar Jan 18, 2023

revolter Jan 18, 2023

revolter Jan 18, 2023

ObjectiveCesar Jan 19, 2023

revolter Jan 19, 2023

rogerluan left a comment

rogerluan left a comment

revolter commented Jan 31, 2023

rogerluan left a comment

[ensure_git_status_clean] fix incorrect "ignored" param handling #20976

[ensure_git_status_clean] fix incorrect "ignored" param handling #20976

Conversation

revolter commented Jan 7, 2023 • edited

Checklist

Motivation and Context

Description

Testing Steps

ObjectiveCesar Jan 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ObjectiveCesar Jan 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan left a comment

Choose a reason for hiding this comment

rogerluan left a comment

Choose a reason for hiding this comment

revolter commented Jan 31, 2023

rogerluan left a comment

Choose a reason for hiding this comment

revolter commented Jan 7, 2023 •

edited

ObjectiveCesar Jan 17, 2023 •

edited

ObjectiveCesar Jan 17, 2023 •

edited