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

Introduce autoformatting bot #5065

Merged
merged 1 commit into from Dec 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 9 additions & 7 deletions .github/workflows/README.md
Expand Up @@ -10,10 +10,12 @@

## Automation

| File | Role |
| :-------------------------- | :---------------------------------------------------------- |
| `labeling.yml` | Automatically apply labels on issues and PRs |
| `notify-dco-failure.yml` | Notify a DCO check failure |
| `notify-dco-failure.js` | The main script of the `notify-dco-failure.yml` workflow |
| `release-note-category.yml` | Validate a release-note category label is applied on a PR |
| `release-note-category.js` | The main script of the `release-note-category.yml` workflow |
| File | Role |
| :-------------------------- | :------------------------------------------------------------- |
| `autoformat.yml` | Apply autoformatting when a PR is commented with `autoformat` |
| `autoformat.js` | Define utility functions used in the `autoformat.yml` workflow |
| `labeling.yml` | Automatically apply labels on issues and PRs |
| `notify-dco-failure.yml` | Notify a DCO check failure |
| `notify-dco-failure.js` | The main script of the `notify-dco-failure.yml` workflow |
| `release-note-category.yml` | Validate a release-note category label is applied on a PR |
| `release-note-category.js` | The main script of the `release-note-category.yml` workflow |
39 changes: 39 additions & 0 deletions .github/workflows/autoformat.js
@@ -0,0 +1,39 @@
const createCommitStatus = async (context, github, sha, state) => {
const { workflow, runId } = context;
const { owner, repo } = context.repo;
const target_url = `https://github.com/${owner}/${repo}/actions/runs/${runId}`;
await github.repos.createCommitStatus({
owner,
repo,
sha,
state,
target_url,
description: sha,
context: workflow,
});
};

const createStatus = async (context, github) => {
const { owner, repo } = context.repo;
const pull_number = context.issue.number;
const pr = await github.pulls.get({ owner, repo, pull_number });
const { sha, ref } = pr.data.head;
await createCommitStatus(context, github, sha, 'pending');
return {
repository: `${owner}/${repo}`,
pull_number,
sha,
ref,
};
};

const updateStatus = async (context, github, sha, needs) => {
const failed = Object.values(needs).some(({ result }) => result === 'failure');
const state = failed ? 'failure' : 'success';
await createCommitStatus(context, github, sha, state);
};

module.exports = {
createStatus,
updateStatus,
};
194 changes: 194 additions & 0 deletions .github/workflows/autoformat.yml
@@ -0,0 +1,194 @@
# A workflow to apply autoformatting when a PR is commented with 'autoformat'.

name: Autoformat
on:
issue_comment:
types: [created, edited]

defaults:
run:
shell: bash --noprofile --norc -exo pipefail {0}

jobs:
check-comment:
runs-on: ubuntu-latest
if: ${{ github.event.issue.pull_request && contains(github.event.comment.body, 'autoformat') }}
outputs:
matched: ${{ steps.check-comment.outputs.result }}
repository: ${{ fromJSON(steps.create-status.outputs.result).repository }}
ref: ${{ fromJSON(steps.create-status.outputs.result).ref }}
sha: ${{ fromJSON(steps.create-status.outputs.result).sha }}
pull_number: ${{ fromJSON(steps.create-status.outputs.result).pull_number }}
steps:
- uses: actions/checkout@v2
- name: Check comment
id: check-comment
uses: actions/github-script@v4
with:
result-encoding: string
script: |
return context.payload.comment.body.trim() === 'autoformat';
- name: Create status
id: create-status
if: ${{ steps.check-comment.outputs.result == 'true' }}
uses: actions/github-script@v4
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const autoformat = require('./.github/workflows/autoformat.js');
return await autoformat.createStatus(context, github);

check-diff:
runs-on: ubuntu-latest
needs: check-comment
if: ${{ needs.check-comment.outputs.matched == 'true' }}
outputs:
py_changed: ${{ steps.check-diff.outputs.py_changed }}
ui_changed: ${{ steps.check-diff.outputs.ui_changed }}
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
with:
python-version: '3.6'
- run: |
pip install requests
- name: Check diff
id: check-diff
run: |
repository="${{ needs.check-comment.outputs.repository }}"
pull_number="${{ needs.check-comment.outputs.pull_number }}"
changed_files="$(python dev/list_changed_files.py --repository $repository --pr-num $pull_number)"
py_changed=$([[ -z $(echo "$changed_files" | grep '\.py$') ]] && echo "false" || echo "true")
ui_changed=$([[ -z $(echo "$changed_files" | grep '^mlflow/server/js') ]] && echo "false" || echo "true")
echo "::set-output name=py_changed::$py_changed"
echo "::set-output name=ui_changed::$ui_changed"

python:
# Generate a patch to format python files.
runs-on: ubuntu-latest
needs: [check-comment, check-diff]
if: ${{ needs.check-diff.outputs.py_changed == 'true' }}
outputs:
has_diff: ${{ steps.check-diff.outputs.has_diff }}
steps:
- uses: actions/checkout@v2
with:
repository: ${{ needs.check-comment.outputs.repository }}
ref: ${{ needs.check-comment.outputs.ref }}
- uses: actions/setup-python@v2
with:
python-version: '3.6'
- name: Install dependencies
run: |
pip install -r requirements/lint-requirements.txt
- name: Run black
run: |
black .
- name: Check diff
id: check-diff
run: |
git diff --output=python.diff
has_diff=$([[ -z "$(cat python.diff)" ]] && echo "false" || echo "true")
echo "::set-output name=has_diff::$has_diff"
Copy link
Member

Choose a reason for hiding this comment

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

Did you want to include pylint checks from the repo's pylintrc for only the diff files as well to fast-fail since full linting checks take quite a bit of time to run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment! The role of this bot is to fix something auto-fixable, pylint errors are (usually) not auto-fixable.

Copy link
Member

Choose a reason for hiding this comment

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

With the local commit hook, I guess that would be redundant (you have to fix lint issues on diffs before a commit occurs anyway). This makes perfect sense to leave it solely up to black for automated adjustments.

- uses: actions/upload-artifact@v2
if: ${{ steps.check-diff.outputs.has_diff == 'true' }}
with:
name: python.${{ github.run_id }}.diff
path: python.diff
if-no-files-found: error
retention-days: 1

ui:
# Generate a patch to format files for MLflow UI.
runs-on: ubuntu-latest
needs: [check-comment, check-diff]
if: ${{ needs.check-diff.outputs.ui_changed == 'true' }}
outputs:
has_diff: ${{ steps.check-diff.outputs.has_diff }}
defaults:
run:
working-directory: mlflow/server/js
steps:
- uses: actions/checkout@v2
with:
repository: ${{ needs.check-comment.outputs.repository }}
ref: ${{ needs.check-comment.outputs.ref }}
- uses: actions/setup-node@v1
with:
node-version: 10.x
- name: Install dependencies
run: |
npm i
- run: |
npm run lint:fix
Copy link
Member

Choose a reason for hiding this comment

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

this is calling .eslintrc.js settings as defined in package.json operating solely on mlflow.server.js.src, right? So we're only looking at .js, .jsx, .ts, .tsx modules in src?
I'm likely completely wrong about this, but does jest's "collectCoverageFrom" apply to the lint:fix script execution to restrict auto-formatting lint corrections based on jest, or does it need to be explicitly defined in the lint:fix script config e.g., "lint:fix": "eslint src --fix --ext .js,.jsx,.ts,.tsx"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@BenWilson2

we're only looking at .js, .jsx, .ts, .tsx modules in src?

Yes.

does jest's "collectCoverageFrom" apply to the lint:fix script execution

No, it doesn't.

- run: |
npm run extract-i18n
- name: Check diff
id: check-diff
run: |
git diff --output=ui.diff
has_diff=$([[ -z "$(cat ui.diff)" ]] && echo "false" || echo "true")
echo "::set-output name=has_diff::$has_diff"
- uses: actions/upload-artifact@v2
if: ${{ steps.check-diff.outputs.has_diff == 'true' }}
with:
name: ui.${{ github.run_id }}.diff
path: mlflow/server/js/ui.diff
if-no-files-found: error
retention-days: 1

apply-patches:
# Apply the patches and commit changes to the PR branch.
runs-on: ubuntu-latest
needs: [check-comment, check-diff, python, ui]
if: |
always() &&
(needs.python.result == 'success' && needs.python.outputs.has_diff == 'true') ||
(needs.ui.result == 'success' && needs.ui.outputs.has_diff == 'true')
steps:
- uses: actions/checkout@v2
with:
repository: ${{ needs.check-comment.outputs.repository }}
ref: ${{ needs.check-comment.outputs.ref }}
# As described in the doc below, if we use `secrets.GITHUB_TOKEN`, a commit created by
# this workflow will not trigger other workflows:
# https://docs.github.com/en/actions/security-guides/automatic-token-authentication#using-the-github_token-in-a-workflow
# To make it work, commit changes using the mlflow-automation bot (https://github.com/mlflow-automation).
token: ${{ secrets.MLFLOW_AUTOMATION_TOKEN }}
- uses: actions/download-artifact@v2
if: ${{ needs.python.result == 'success' && needs.python.outputs.has_diff == 'true' }}
with:
name: python.${{ github.run_id }}.diff
path: /tmp/patches
- uses: actions/download-artifact@v2
if: ${{ needs.ui.result == 'success' && needs.ui.outputs.has_diff == 'true' }}
with:
name: ui.${{ github.run_id }}.diff
path: /tmp/patches
- name: Apply patches
run: |
find /tmp/patches -maxdepth 1 -type f -name '*.diff' | xargs git apply --verbose
git diff
- name: Commit changes
run: |
git config --global user.name 'mlflow-automation'
git config --global user.email 'mlflow-automation@users.noreply.github.com'
run_url="https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}"
git commit -sam "Autoformat: $run_url"
git push

update-status:
runs-on: ubuntu-latest
needs: [check-comment, check-diff, python, ui, apply-patches]
if: ${{ always() && needs.check-comment.outputs.matched == 'true' }}
steps:
- uses: actions/checkout@v2
- name: Update status
uses: actions/github-script@v4
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const needs = ${{ toJson(needs) }};
const sha = '${{ needs.check-comment.outputs.sha }}'
const autoformat = require('./.github/workflows/autoformat.js');
await autoformat.updateStatus(context, github, sha, needs);
6 changes: 4 additions & 2 deletions dev/lint.sh
Expand Up @@ -10,8 +10,10 @@ echo -e "\n========== black ==========\n"
black --check .

if [ $? -ne 0 ]; then
echo 'Run this command to apply Black formatting:'
echo '$ pip install $(cat requirements/lint-requirements.txt | grep "black==") && black .'
echo '
To apply black foramtting, do one of the following:
- Run `pip install $(cat requirements/lint-requirements.txt | grep "^black==") && black .`
- Comment `autoformat` on the PR'
fi

echo -e "\n========== pylint ==========\n"
Expand Down