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

PBE-1524: automatically convert typescript to javascript in pre-commit #48

Conversation

canzhen
Copy link

@canzhen canzhen commented Apr 17, 2024

Description

Fixes #PBE-1524

Changes

Add pre-commit hook to convert typescript to javascript for k6 test script for load testing purpose.

🚀 PR created with fotingo

@canzhen canzhen requested a review from a team as a code owner April 17, 2024 22:26
@canzhen canzhen requested review from MaximF and rmuralis April 17, 2024 22:26
if git diff-tree --no-commit-id --name-only -r HEAD..origin/$currentBranch | grep -q 'load-test/'; then
echo 'hahahha!'
cd load-test
npm run bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the pre-commit hook to be that specific? Only looking at load test. I wonder if there is an existing pre-commit hook to run if there is a pacakge.json in the repo? Our standard is to have a build script in the pacakge.json instead of bundle

Copy link
Contributor

Choose a reason for hiding this comment

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

This might fail when running in CI since you would first have to ensure there is an npm install

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since this is only used in 1 repo today, you could define the pre-commit hook local to that repo until we know how to standardize this?

Copy link
Author

Choose a reason for hiding this comment

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

@tagoro9 Good point, I added npm install. And I updated the script from bundle to build. I tested in the vehicle-search repo and it worked.

I wonder if there is an existing pre-commit hook to run if there is a pacakge.json in the repo?
Not sure if I understand your question, but for vehicle-search, there is a package.json in the root level. But within load-test/ folder there's its own package.json file for converting typescript and other stuff.

I've posted a thread in #proj-load-testing channel discussing about different approaches of this ts -> js automation. The other way is to directly commit to the .git/hooks but after some research it didn't seem like we can commit anything under .git, it's auto ignored by git. Happy to hear your thoughts!

Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-commit lets you create hooks local to a repo without committing that folder

Copy link
Author

@canzhen canzhen Apr 17, 2024

Choose a reason for hiding this comment

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

Yes but I don't think the existing pre-commit doesn't do the converting part?

Edit: conversations were moved to slack channel. will close the PR because we haven’t standardized any of the load testing patterns to make it reusable across repos yet, so probably directly updating .pre-commit-config.yaml makes more sense other than creating a new hook and re-use it across repos.

@canzhen canzhen force-pushed the f/pbe-1524_automatically_convert_typescript_to_javascript_in_pre-commit branch from dcdcabb to 25308d3 Compare April 17, 2024 22:44
@canzhen canzhen closed this Apr 17, 2024
@canzhen canzhen deleted the f/pbe-1524_automatically_convert_typescript_to_javascript_in_pre-commit branch April 17, 2024 22:48
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.

2 participants