-
Notifications
You must be signed in to change notification settings - Fork 286
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
fix(custom-checks): broken package.json sort verification #2434
Conversation
Thankyou for the PR @rwat17 ! |
@jagpreetsinghsasan Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwat17 Please check out my write-up here and then let me know your thoughts! (e.g. pass it back for review)
#2356 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwat17 Are you still working on this?
@petermetz sorry, I was offline for one week. As u pointed in conversation that not using bash to sort packages would be better I made some changes and the check-package-json-sort.ts now works fine. Changed the output of CI github workflow to run "npm run custom-checks" if packages are not sorted. The settings I used to format JSON files should coincide repo guidelines to prevent white space in commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petermetz sorry, I was offline for one week.
@rwat17 No worries at all! Thank you for keeping up the good work!
The settings I used to format JSON files should coincide repo guidelines to prevent white space in commits.
Thank you so much for this specifically. It's a common mistake that we end up mangling the manifest files (package.json files) because the lack of consistent formatting.
As u pointed in conversation that not using bash to sort packages would be better I made some changes and the check-package-json-sort.ts now works fine. Changed the output of CI github workflow to run "npm run custom-checks" if packages are not sorted.
We are getting closer, but the thing is, the shell script's logic of "call yarn custom-checks IF not sorted" is not much use because it just ends up performing the sort on the CI server's local filesystem, but that is a temporary server instance in the cloud that is gone after the execution. What needs to happen instead is that the contributor will run the sort locally and then commit the changes themselves (so that we don't have to go down the rabbit hole of giving commit rights to the CI and also dealing with the issue of how we review those changes made by the robots on our behalf)
The other issue right now is that yarn custom-checks
is mean to fail if there is an issue by default, not to fix issues (same thought process as above) but right now it still technically succeeds because you are not putting anything in the errors
array that would get returned to the callin script.
With that said, I agree we should provide the tooling for contributors to easily fix the sorting (I also do not have time to manually sort my package.json files and I wouldn't expect anyone else to do that either).
So, based on all that my specific suggestions here are:
- The way/logic
./tools/custom-checks/check-package-json-sort.ts
file works right now should be moved to another file as a convenience script, such as./tools/sort-package-jsons.ts
which would be a standalone utility that can be called by contributors with the outcome of the sorting having been performed for them. (Notice the naming difference where I droppe thecheck
part indicating that this is a script that does something to the codebase instead of verifying/checking it as-is. - Then refactor
./tools/custom-checks/check-package-json-sort.ts
so that it does only the checking, e.g. after it's execution it has theerrors
array populated with an entry for every single package.json file (with it's path) that had issues with sorting. That way contributors can see what the problem was in the CI logs and then they can easily fix it by running the utility script from1)
on their local machineand then commit the changes themselves, keeping the review process intact. - Remove the shell script and the new job added altogether because it is completely redundant after the changes you've just made in
1)
and2)
(there is a job in ci.yaml that runsyarn custom-checks
that will start failing if you implement2)
. - Refactor the default glob pattern to be pulled from the
lerna.json
file directly. This way you don't have to solve the problem that we now have bothcacti
andcactus
prefixed package names (which is not being handled by either scripts at the moment).
You can look at this file for inspiration (mostly copy-paste-able) =>tools/custom-checks/check-pkg-npm-scope.ts
specifically how the import happens and then it is used for globbing:
import lernaCfg from "../../lerna.json" assert { type: "json" };
cdd9978
to
b3fa9d5
Compare
@petermetz please review and confirm it matches your requirements from last comment :) I'm going to refactor it tomorrow. |
b3fa9d5
to
94ed3b5
Compare
@rwat17 Slightly off-topic but I highly recommend using the VSCode prettier and eslint extensions, they let you fix the formatting problems instantly 99% of the time with this option in the quick-fix menu of the editor (see |
This is necessary because in another commit we are introducing the new custom check that does verify sortedness and does (correctly) fail if any of the package.json files are not sorted as they should be. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
- Remove not working check-package-json-sort.ts script. - Add ci action to check for unsorted package.json files. Peter's changes: 1. Changed the commit message in a way that it won't show up in the release notes among the bug fixes (the section that is meant for production bug-fixes not tooling bug-fixes) 2. Fixed typos in the .ts scripts 3. Applied the prettier/eslint formatting, deleted unused variables 4. Added a convenience script to the root package.json that runs the package.json sorter 5. Fixed the sorter script so that it appends a newline character after the sorted JSON string when writing back to the file. This is needed because when you run `yarn install` or `npm install` they both like to close the package.json file's with an empty line last so we have to mimic that in order to avoid spamming all future PRs with these line changes. One problem that this might still have is that on different operating systems the newline character might be different and it also depends on the git configuration in effect (you can configure Windows' git to use Unix line endings for example). So it's not trivial how to fix it but we'll cross that bridge when we get to it. Closes: hyperledger-cacti#2356 Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: Tomasz Awramski <tomasz.awramski@fujitsu.com Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
94ed3b5
to
71a3b1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwat17 LGTM, thank you very much for the updates!
FYI: I made a few minor tweaks to make it pass the linter and the CI checks (see the updated commit message).
Peter's changes:
release notes among the bug fixes (the section that is meant for production
bug-fixes not tooling bug-fixes)
package.json sorter
the sorted JSON string when writing back to the file. This is needed
because when you run
yarn install
ornpm install
they both like toclose the package.json file's with an empty line last so we have to mimic
that in order to avoid spamming all future PRs with these line changes.
One problem that this might still have is that on different operating
systems the newline character might be different and it also depends on
the git configuration in effect (you can configure Windows' git to use
Unix line endings for example). So it's not trivial how to fix it but
we'll cross that bridge when we get to it.
Closes: #2356
Co-authored-by: Peter Somogyvari peter.somogyvari@accenture.com
Signed-off-by: Tomasz Awramski <tomasz.awramski@fujitsu.com
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com