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

infra: add CI step to detect generate script diffs #1405

Merged
merged 16 commits into from
Oct 9, 2022

Conversation

sunadoi
Copy link
Contributor

@sunadoi sunadoi commented Oct 2, 2022

#813

After running the generate:locales command in the workflow/ci pipeline, check for some uncommitted diffs with the following command.

git add .
git diff --cached --exit-code
  • The --exit-code option causes it to fail if there are any diffs.

I tested this CI on my repository.
It seems to work well.
ref: https://github.com/sunadoi/faker/actions/runs/3164634036/jobs/5153063988

@sunadoi sunadoi requested a review from a team as a code owner October 2, 2022 02:16
@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #1405 (ada8798) into main (13dac4f) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1405   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files        2166     2166           
  Lines      237442   237442           
  Branches     1040     1040           
=======================================
  Hits       236535   236535           
  Misses        886      886           
  Partials       21       21           

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

This does not detect deleted files, but I'm not sure whether this is necessary.

@ST-DDT ST-DDT requested review from a team October 2, 2022 10:24
@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug c: infra Changes to our infrastructure or project setup labels Oct 2, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Oct 2, 2022
@ST-DDT ST-DDT linked an issue Oct 2, 2022 that may be closed by this pull request
@sunadoi
Copy link
Contributor Author

sunadoi commented Oct 2, 2022

This does not detect deleted files

I didn't give it enough thought.
How about checking git diff before and after git add?
The former checks for file updates/deletions, the latter checks for file creation.

git diff --exit-code
git add -N .
git diff --exit-code

@sunadoi
Copy link
Contributor Author

sunadoi commented Oct 2, 2022

To be able to detect deleted files, the method of checking for diffs has been changed to the following command.

git add .
git diff --cached --exit-code

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Oct 3, 2022

I tried to run it in my fork and it doesn't work:
https://github.com/ST-DDT/faker/actions/runs/3175539763/jobs/5173706263#step:8:17

You might need the PR write permission for that action.
https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

Please assign it for the lowest possible scope (e.g. only for this single job).

@sunadoi
Copy link
Contributor Author

sunadoi commented Oct 4, 2022

Please assign it for the lowest possible scope (e.g. only for this single job).

I gave permissions pull-requests: write in this job only, and it seems to work in my fork.
https://github.com/sunadoi/faker/actions/runs/3179609661/jobs/5182304903

ST-DDT
ST-DDT previously approved these changes Oct 4, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

@Shinigami92 Is these Jsdocs what you intended with your request?
Maybe you could give an example of what you would like to have.

.github/workflows/commentCodeGeneration.js Outdated Show resolved Hide resolved
.github/workflows/commentCodeGeneration.js Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Oct 9, 2022
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Beside that, I think we are good to go 👍
So I will already approve so other maintainers know that it counts after you fixed the dep version

package.json Outdated Show resolved Hide resolved
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Do we have to .gitginore the transpiled .js script?

  • If yes, why is the script not failing?
  • If not, why not?

.github/workflows/commentCodeGeneration.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Oct 9, 2022

Do we have to .gitginore the transpiled .js script?

* If yes, why is the script not failing?

* If not, why not?

Ah I see, because it is transpiled only after the check.
Please add *.js files in the workflows folder to the gitignore anyway, in case somebody transpiles it locally.

(I plan to use your script as a template to automate some more tasks 😻 )

@sunadoi
Copy link
Contributor Author

sunadoi commented Oct 9, 2022

(I plan to use your script as a template to automate some more tasks 😻 )

That sounds nice!
I'm so happy to hear that.😘

@ST-DDT ST-DDT requested review from Shinigami92 and a team October 9, 2022 15:19
@ST-DDT ST-DDT changed the title infra: add CI step to detect generate:locales diffs infra: add CI step to detect generate script diffs Oct 9, 2022
@ST-DDT ST-DDT enabled auto-merge (squash) October 9, 2022 19:28
@ST-DDT ST-DDT merged commit f934792 into faker-js:main Oct 9, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Oct 9, 2022

@sunadoi It looks like this breaks the main branch.
Could you move the job to a new file e.g. pr.yml and run it only for PRs and not main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add CI steps to test execution of generate scripts
3 participants