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

Support fetching without the --progress option #1067

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

simonbaird
Copy link
Contributor

@simonbaird simonbaird commented Jan 5, 2023

If this is merged, setting the progress option to false in the with section of the workflow step will cause git fetch to run without the --progress flag.

Note that the default value of this new option is true, which matches the current behavior, so this PR introduces no behavior change unless the new option is explicitly present and set to false.

The motivation is to be able to suppress the noisy progress status output which adds many hundreds of remote: Counting objects: 85% (386/453) and similar lines in the workflow log.

This should be sufficient to resolve #894 and its older friends, though the solution is different to the one proposed there because it doesn't use the --quiet flag. IIUC git doesn't show the progress status by default when running on GitHub since the output is not a terminal. Adding the --progress flag is what forces the progress output to be shown, so that's why removing it is all that's needed.

Adding the --quiet flag doesn't make a lot of difference once the --progress flag is removed, and actually I think using --quiet would suppress some other more useful output that would be better left visible.

@simonbaird simonbaird requested a review from a team as a code owner January 5, 2023 05:14
@simonbaird
Copy link
Contributor Author

I'm using it here and it seems to work well.

@simonbaird
Copy link
Contributor Author

simonbaird commented Jan 5, 2023

This workflow run shows it working with the --progress option set which is still the default behavior.

@simonbaird
Copy link
Contributor Author

@TingluoHuang would this be something you'd consider taking a look at? I'm looking at your comments from 2019 on #18.

@yeikel
Copy link

yeikel commented May 3, 2023

What is blocking this change?

I am looking forward to seeing this released as well

@simonbaird simonbaird force-pushed the no-progress-option branch from 87596e0 to 3327cce Compare May 4, 2023 19:29
@simonbaird
Copy link
Contributor Author

Rebased on upstream/main, (currently f095bcc).

@simonbaird
Copy link
Contributor Author

simonbaird commented May 4, 2023

Passes formatting and lint checks for me locally:

$ for t in build format-check lint; do npm run $t; done

> checkout@3.5.2 build
> tsc && ncc build && node lib/misc/generate-docs.js

ncc: Version 0.36.1
ncc: Compiling file index.js into CJS
877kB  dist/index.js
877kB  [1343ms] - ncc 0.36.1

> checkout@3.5.2 format-check
> prettier --check '**/*.ts'

Checking formatting...
All matched files use Prettier code style!

> checkout@3.5.2 lint
> eslint src/**/*.ts

And tests:

$ npm test
...
Test Suites: 7 passed, 7 total
Tests:       68 passed, 68 total
Snapshots:   0 total
Time:        7.829 s, estimated 8 s
Ran all test suites.

@simonbaird
Copy link
Contributor Author

simonbaird commented May 5, 2023

I reworded the PR description and added some additional explanations.

@simonbaird
Copy link
Contributor Author

@ericsciple WDYT?

@simonbaird simonbaird force-pushed the no-progress-option branch from 3327cce to ce4272e Compare July 25, 2023 20:38
@simonbaird
Copy link
Contributor Author

Rebased on 96f5310.

@simonbaird simonbaird force-pushed the no-progress-option branch from ce4272e to d053b2f Compare July 25, 2023 20:44
@simonbaird
Copy link
Contributor Author

$ for t in build format-check lint; do npm run $t; done

> checkout@3.5.3 build
> tsc && ncc build && node lib/misc/generate-docs.js

ncc: Version 0.36.1
ncc: Compiling file index.js into CJS
879kB  dist/index.js
879kB  [1112ms] - ncc 0.36.1

> checkout@3.5.3 format-check
> prettier --check '**/*.ts'

Checking formatting...
All matched files use Prettier code style!

> checkout@3.5.3 lint
> eslint src/**/*.ts

@bf
Copy link

bf commented Aug 2, 2023

Guys, what is blocking this? Would be really nice to have. Currently the checkout pollutes my logs.

@slang25
Copy link

slang25 commented Aug 30, 2023

@luketomlinson are you able to comment, there are a lot of people wanting this.

@yeikel
Copy link

yeikel commented Aug 30, 2023

Well, currently there are conflicts. It would help if we can resolve these first. Otherwise it'll take more time before this is looked at again

@simonbaird Could you please work on that?

@simonbaird
Copy link
Contributor Author

I'll give it another rebase.

Setting the `progress` option to false in the `with` section of the
workflow step will cause git fetch to run without `--progress`.

The motivation is to be able to suppress the noisy progress status
output which adds many hundreds of "remote: Counting objects: 85%
(386/453)" and similar lines in the workflow log.

This should be sufficient to resolve actions#894 and its older friends,
though the solution is different to the one proposed there because
it doesn't use the --quiet flag. IIUC git doesn't show the progress
status by default since the output is not a terminal, so that's why
removing the --progress option is all that's needed.

Adding the --quiet flag doesn't make a lot of difference once the
--progress flag is removed, and actually I think using --quiet would
suppress some other more useful output that would be better left
visible.

Signed-off-by: Simon Baird <sbaird@redhat.com>
@simonbaird
Copy link
Contributor Author

Rebased on upstream/main (currently at 97a652b). Now includes some test coverage 😎 .

showProgress: true
}

await git.fetch(refSpec, options)
Copy link
Contributor Author

@simonbaird simonbaird Aug 30, 2023

Choose a reason for hiding this comment

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

It might be nice to merge these options onto the default option values here so the assertions become more like real world examples. (This is just an idea for a separate PR though.)

],
expect.any(Object)
)
})
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'd be happy to remove some of the test coverage if it's considered too much. I was trying to follow the example set in #579 .

Copy link

@yeikel yeikel Aug 31, 2023

Choose a reason for hiding this comment

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

Too much coverage is rarely a problem. Thank you for the detailed effort!

Copy link
Contributor

@vanZeben vanZeben left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @simonbaird, especially with the comprehensive test coverage! 🎉 I'll cut a release for this later today

@vanZeben vanZeben merged commit 8b5e8b7 into actions:main Sep 1, 2023
@vanZeben
Copy link
Contributor

vanZeben commented Sep 1, 2023

I lied, checking internally, we're planning on cutting a new Major version release next week to include the recent node20 bump, that major release will include this too, so look forward to v4.0.0 for this change!

gitfool added a commit to gitfool/cake-docker that referenced this pull request Oct 21, 2023
gitfool added a commit to gitfool/Cake.Dungeon that referenced this pull request Oct 21, 2023
gitfool added a commit to gitfool/GitHubActions.Dungeon that referenced this pull request Oct 21, 2023
myparcel-bot bot added a commit to myparcelnl/actions that referenced this pull request Oct 24, 2023
gitfool added a commit to gitfool/Cake.Dungeon that referenced this pull request Jan 7, 2024
gitfool added a commit to gitfool/BoardGameGeek.Dungeon that referenced this pull request Jan 7, 2024
gitfool added a commit to gitfool/Pulumi.Dungeon that referenced this pull request Jan 13, 2024
gitfool added a commit to gitfool/Pulumi.Dungeon that referenced this pull request Jan 13, 2024
link2xt added a commit to chatmail/core that referenced this pull request Feb 15, 2024
Also disable --progress.
It is not disabled by default for backward compatibility,
but solves the problem of lots of progress lines
in the downloadable raw output:
actions/checkout#1067
link2xt added a commit to chatmail/core that referenced this pull request Feb 15, 2024
Also disable --progress.
It is not disabled by default for backward compatibility,
but solves the problem of lots of progress lines
in the downloadable raw output:
actions/checkout#1067
link2xt added a commit to chatmail/core that referenced this pull request Feb 17, 2024
Also disable --progress.
It is not disabled by default for backward compatibility,
but solves the problem of lots of progress lines
in the downloadable raw output:
actions/checkout#1067
link2xt added a commit to chatmail/core that referenced this pull request Feb 17, 2024
Also disable --progress.
It is not disabled by default for backward compatibility,
but solves the problem of lots of progress lines
in the downloadable raw output:
actions/checkout#1067
github-actions bot pushed a commit to capitnflam/eslint-plugin that referenced this pull request Mar 15, 2024
## [1.0.1](v1.0.0...v1.0.1) (2024-03-15)

### chore

* dependabot updates setup ([#2](#2)) ([7e71810](7e71810))
* **deps-dev:** bump [@commitlint](https://github.com/commitlint)/cli from 19.1.0 to 19.2.0 ([#5](#5)) ([737d89b](737d89b))
* **deps-dev:** bump [@types](https://github.com/types)/node from 20.11.26 to 20.11.27 ([#8](#8)) ([ba41253](ba41253))
* **deps-dev:** bump semantic-release from 23.0.2 to 23.0.3 ([#6](#6)) ([7d29884](7d29884))
* **deps:** bump actions/checkout from 2 to 4 ([#4](#4)) ([f35995a](f35995a)), closes [actions/checkout#1436](actions/checkout#1436) [actions/checkout#1067](actions/checkout#1067) [actions/checkout#1447](actions/checkout#1447) [actions/checkout#1436](actions/checkout#1436) [actions/checkout#1067](actions/checkout#1067) [actions/checkout#1377](actions/checkout#1377) [actions/checkout#579](actions/checkout#579) [actions/checkout#1437](actions/checkout#1437) [actions/checkout#579](actions/checkout#579) [actions/checkout#1437](actions/checkout#1437) [actions/checkout#1196](actions/checkout#1196) [actions/checkout#1287](actions/checkout#1287) [actions/checkout#1369](actions/checkout#1369) [actions/checkout#1376](actions/checkout#1376) [actions/checkout#1196](actions/checkout#1196) [actions/checkout#1287](actions/checkout#1287) [actions/checkout#1369](actions/checkout#1369) [actions/checkout#1289](actions/checkout#1289) [#1286](https://github.com/capitnflam/eslint-plugin/issues/1286) [actions/checkout#1246](actions/checkout#1246) [actions/checkout#1246](actions/checkout#1246) [actions/checkout#1598](actions/checkout#1598) [actions/checkout#1511](actions/checkout#1511) [actions/checkout#1514](actions/checkout#1514) [#770](https://github.com/capitnflam/eslint-plugin/issues/770) [actions/checkout#1057](actions/checkout#1057) [#1514](https://github.com/capitnflam/eslint-plugin/issues/1514) [#1511](https://github.com/capitnflam/eslint-plugin/issues/1511) [#1510](https://github.com/capitnflam/eslint-plugin/issues/1510) [#1496](https://github.com/capitnflam/eslint-plugin/issues/1496) [#1396](https://github.com/capitnflam/eslint-plugin/issues/1396) [#1452](https://github.com/capitnflam/eslint-plugin/issues/1452) [#1447](https://github.com/capitnflam/eslint-plugin/issues/1447) [#1067](https://github.com/capitnflam/eslint-plugin/issues/1067) [#1436](https://github.com/capitnflam/eslint-plugin/issues/1436) [#1437](https://github.com/capitnflam/eslint-plugin/issues/1437)
* **deps:** bump actions/setup-node from 2 to 4 ([#3](#3)) ([b003245](b003245)), closes [actions/setup-node#866](actions/setup-node#866) [actions/setup-node#868](actions/setup-node#868) [actions/setup-node#876](actions/setup-node#876) [actions/setup-node#868](actions/setup-node#868) [actions/setup-node#861](actions/setup-node#861) [actions/setup-node#859](actions/setup-node#859) [actions/setup-node#870](actions/setup-node#870) [actions/setup-node#872](actions/setup-node#872) [actions/setup-node#875](actions/setup-node#875) [actions/setup-node#831](actions/setup-node#831) [actions/setup-node#803](actions/setup-node#803) [actions/setup-node#809](actions/setup-node#809) [actions/setup-node#816](actions/setup-node#816) [actions/setup-node#794](actions/setup-node#794) [actions/setup-node#710](actions/setup-node#710) [actions/setup-node#812](actions/setup-node#812) [actions/setup-node#808](actions/setup-node#808) [actions/setup-node#804](actions/setup-node#804) [actions/setup-node#802](actions/setup-node#802) [actions/setup-node#807](actions/setup-node#807) [#927](https://github.com/capitnflam/eslint-plugin/issues/927) [#921](https://github.com/capitnflam/eslint-plugin/issues/921) [#865](https://github.com/capitnflam/eslint-plugin/issues/865) [#879](https://github.com/capitnflam/eslint-plugin/issues/879) [#898](https://github.com/capitnflam/eslint-plugin/issues/898) [#917](https://github.com/capitnflam/eslint-plugin/issues/917) [#889](https://github.com/capitnflam/eslint-plugin/issues/889) [#884](https://github.com/capitnflam/eslint-plugin/issues/884) [#882](https://github.com/capitnflam/eslint-plugin/issues/882) [#876](https://github.com/capitnflam/eslint-plugin/issues/876)
* **deps:** bump eslint-plugin-react from 7.33.2 to 7.34.0 ([#7](#7)) ([cc5ea82](cc5ea82))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run git fetch with --quiet by default
6 participants