Initial support for pixel registry #6878
Conversation
ladamski
force-pushed
the
la/pixel-schema
branch
from
e1030b3 to
700ea42
Compare
This reverts commit 7e35fd0.
| # Lint step for push (fail on error), for PRs run fix instead | ||
| - name: Check or Fix lint | ||
| id: lint | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| npm run lint.fix || true | ||
| else | ||
| npm run lint | ||
| fi | ||
| working-directory: PixelDefinitions | ||
|
|
||
| # Commit and push changes if lint.fix was run and made changes | ||
| - name: Commit and push linter fixes (PRs only) | ||
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| git config --global user.name "github-actions[bot]" | ||
| git config --global user.email "github-actions[bot]@users.noreply.github.com" | ||
| git add PixelDefinitions/ | ||
| if ! git diff --cached --quiet; then | ||
| git commit -m "chore: auto-fix lint errors" | ||
| git push | ||
| fi |
There was a problem hiding this comment.
Let's check with the Android team if they are ok with automated commits. Windows opted into this, but Apple preferred a simple diff output on error: https://app.asana.com/1/137249556945/project/414709148257752/task/1210957466937460?focus=true
There was a problem hiding this comment.
Bringing my comment:
- lint should always run in PRs and develop and should be added to our pre-commit hook (this las bit needs added).
- lint-fix should be run manually by the owner.
- No automated commits anywhere.
|
|
||
| on: | ||
| push: | ||
| branches: [develop] |
There was a problem hiding this comment.
this is not needed, validation should only run on PR commits and not when the PR is merged. The PR won’t be merged if there are lint issues.
There was a problem hiding this comment.
I don't have a personal preference here, other than to note that:
- @marcosholgado seems to have a different opinion in this comment: Initial support for pixel registry #6878 (comment)
- normally running CI on develop is also desirable as issues can crop up post-merge, but since its just pixel linting I personally don't think it will make much difference
There was a problem hiding this comment.
Yeah I think it should run on develop too, wouldn't be the first time we have issues post-merge because of some weird randomness.
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| npm run lint.fix || true | ||
| else | ||
| npm run lint |
There was a problem hiding this comment.
We can do this as part of our https://github.com/duckduckgo/Android/blob/develop/.github/workflows/ci.yml that runs on each PR merge. That would also prevent this workflow from running in parallel.
There was a problem hiding this comment.
My preference would be to have the pixel linter separated out so everything pixel validation related is in one workflow. This matches what we have done on other platforms and will make it easier to then consolidate into a shareable workflow in https://github.com/duckduckgo/native-github-asana-sync once we are more stable.
There was a problem hiding this comment.
Since we aren't actually committing anything, I think it makes sense to run it in parallel? I've also scoped it down to only trigger on
paths:
- 'PixelDefinitions/**'
... so it won't even show up on most PRs
There was a problem hiding this comment.
@marcosholgado , @malmstein really appreciate your feedback here!
As @nshuba mentioned above, our longer term goal is to consolidate the common components across 6+ products into a shared one. As such I'd like to see if the current state is acceptable. To summarize:
- package.json stays in the PixelDefinitions subdirectory
- a separate pixel-validation.yml runs only for PR with commits in the PixelDefinitions subdirectory
- fail on error, no auto fixing / committing
There was a problem hiding this comment.
Re package.json 👍
Re fail on error, no auto fixing / committing 👍
I think it should run on develop too and it should be added to the pre-commit hook too so we don't have to run the whole CI to note that something failed. In fact, I think the pre-commit should run lint.fix and CI only lint.
There was a problem hiding this comment.
@marcosholgado @malmstein I reverted to running against develop, and added a pre-commit for lint.fix.
Please take a look at the latter in particular as I'm not too familiar with the Git pre-commit workflows.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1171671347221384/task/1210907475542624
Description
Initial support for https://app.asana.com/1/137249556945/project/1209805270658160/list/1209805324244987
Specific subtask for this PR: https://app.asana.com/1/137249556945/task/1210907475542626
Steps to test this PR
https://app.asana.com/1/137249556945/task/1209901985192016?focus=true using ".../Android/PixelDefinitions/" as the target directory where specified
Feature 1
UI changes