feat: Only generate manifest if update is specified #8

2020-03-05T04:54:33Z

msrose commented

2020-03-05 04:54:33 +00:00

(Migrated from github.com)

Mostly a suggestion based upon how I imagine packwatch would be used in CI:

In CI: simply run packwatch. It fails if the package size is bigger than expected. The current behaviour is to generate a new manifest and succeed, so if somehow the manifest gets deleted, the build will pass unwittingly, and the generated file won't even get committed to the repository.

By requiring the --update-manifest flag to be passed to generate the manifest for the first time, it ensures that packwatch won't "accidentally" pass in CI if it hasn't already been configured locally already.

Let me know if this makes sense! There are other options but this was the first that came to mind. Also should probably write a test for this somehow 😛

It would be a breaking change so I've added a commit message that our pal semantic-release should pick up.

Mostly a suggestion based upon how I imagine packwatch would be used in CI: In CI: simply run `packwatch`. It fails if the package size is bigger than expected. The current behaviour is to generate a new manifest and succeed, so if somehow the manifest gets deleted, the build will pass unwittingly, and the generated file won't even get committed to the repository. By requiring the `--update-manifest` flag to be passed to generate the manifest for the first time, it ensures that packwatch won't "accidentally" pass in CI if it hasn't already been configured locally already. Let me know if this makes sense! There are other options but this was the first that came to mind. Also should probably write a test for this somehow 😛 It would be a breaking change so I've added a commit message that our pal semantic-release should pick up.

codecov-io commented

2020-03-06 21:20:26 +00:00

(Migrated from github.com)

Codecov Report

Merging #8 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master     #8   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          34     34           
  Branches        5      5           
=====================================
  Hits           34     34

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a34e42a...36884f0. Read the comment docs.

# [Codecov](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=h1) Report > Merging [#8](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=desc) into [master](https://codecov.io/gh/mcataford/packwatch/commit/a34e42a5e15adf80bd0637fbfae3922dd116693b?src=pr&el=desc) will **not change** coverage. > The diff coverage is `n/a`. [![Impacted file tree graph](https://codecov.io/gh/mcataford/packwatch/pull/8/graphs/tree.svg?width=650&token=IVEisJMCje&height=150&src=pr)](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #8 +/- ## ===================================== Coverage 100% 100% ===================================== Files 1 1 Lines 34 34 Branches 5 5 ===================================== Hits 34 34 ``` ------ [Continue to review full report at Codecov](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=footer). Last update [a34e42a...36884f0](https://codecov.io/gh/mcataford/packwatch/pull/8?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mcataford commented

2020-03-06 21:24:41 +00:00

(Migrated from github.com)

I feel that using --update-manifest to generate it initially is a bit weird semantically. However, I agree that CI should fail if you do not have a .packwatch.json file - I would advocate for returning an error code when generating the file for the first time so that CI would always fail if it has to create the file, otherwise it would pas. This would preserve the idea that --update-manifest expects a file to exist already.

I feel that using `--update-manifest` to generate it initially is a bit weird semantically. However, I agree that CI should fail if you do not have a `.packwatch.json` file - I would advocate for returning an error code when generating the file for the first time so that CI would always fail if it has to create the file, otherwise it would pas. This would preserve the idea that `--update-manifest` expects a file to exist already.

👍 1

mcataford (Migrated from github.com) approved these changes 2020-03-08 17:12:33 +00:00

mcataford (Migrated from github.com) left a comment

LGTM

src/index.js

					
				@ -37,1 +36,4 @@

				    // If the update flag wasn't specified, exit with a non-zero code so we

				    // don't "accidentally" pass CI builds if the manifest didn't exist

				    process.exit(isUpdatingManifest ? 0 : 1)

				}

mcataford (Migrated from github.com) commented

2020-03-08 17:12:19 +00:00

An argument could be made that there's never a valid use case in which you'd want CI to run --update-manifest and pass either (since having --update-manifest in CI would always pass the check). That being said, I feel that this can be addressed further up and check if a CI env variable is set to error out early if --update-manifest is passed in.

LGTM.

An argument could be made that there's never a valid use case in which you'd want CI to run `--update-manifest` and pass either (since having `--update-manifest` in CI would always pass the check). That being said, I feel that this can be addressed further up and check if a CI env variable is set to error out early if `--update-manifest` is passed in. LGTM.

mcataford commented

2020-03-08 17:13:55 +00:00

(Migrated from github.com)

@all-contributors please add @msrose for code

allcontributors[bot] commented

2020-03-08 17:14:06 +00:00

(Migrated from github.com)

@mcataford

I've put up a pull request to add @msrose! 🎉

@mcataford I've put up [a pull request](https://github.com/mcataford/packwatch/pull/13) to add @msrose! :tada:

mcataford commented

2020-03-08 17:15:35 +00:00

(Migrated from github.com)

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

:tada: This PR is included in version 2.0.0 :tada: The release is available on: - [npm package (@latest dist-tag)](https://www.npmjs.com/package/packwatch/v/2.0.0) - [GitHub release](https://github.com/mcataford/packwatch/releases/tag/v2.0.0) Your **[semantic-release](https://github.com/semantic-release/semantic-release)** bot :package::rocket:

This repo is archived. You cannot comment on pull requests.

No reviewers

mcataford