Skip to content

Commit

Permalink
[ui] Clean up dagster ui build steps (#19309)
Browse files Browse the repository at this point in the history
## Summary & Motivation

Clean up some of our ts/lint/jest behavior in CI. Hopefully this will
help us fail the build faster when errors arise.

- Add `lint:ci` commands for `app-oss`, `ui-core`, and `ui-components`.
These will raise errors correctly without trying to execute fixes, which
can take unnecessary time.
- For `ui-core`, we're effectively running lint twice (`lint`,
`check-lint`) and prettier three times (once each for the lint, runs,
plus a `check-prettier`). We don't need to do all of this, and this is
one of our more expensive steps. Just run it once.
- Since `ui-core`'s lint step takes around a minute and a half, this
change should shave at least that amount of time off the `dagster-ui`
build. 🥳 (See
https://buildkite.com/dagster/dagster-dagster/builds/74104#018d227a-5167-4bb1-b73a-b8f16db2c173
for an example of the double-linting in action.)
- The cheapest steps should be in `ui-components`, since it's a
relatively small package, and since it has no dependencies on GraphQL
generation. ~Move these to be first so that we can fail fast if they're
broken.~
- **Edit:** Move `ui-components` into its own build step entirely. This
way it will only run when it has changes, which tends to be just a
handful of PRs per month. `ui-core` can skip it entirely, saving a
couple more minutes there.
- ~Similarly, `app-oss` is cheap. Move it above `ui-core` so that it can
fail fast too.~
- **Edit:** Never mind -- moving this back below `ui-core` because
`app-oss` is touched rarely enough that we should fail faster on
`ui-core`.

## How I Tested These Changes

Introduce lint/prettier problems in `ui-components` and `ui-core`.
Verify that the build short-circuits on the failure at the correct step.
  • Loading branch information
hellendag authored Jan 19, 2024
1 parent 18ac1bc commit bfde747
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
from typing import List, Optional

from dagster_buildkite.steps.dagster import build_dagster_steps, build_repo_wide_steps
from dagster_buildkite.steps.dagster_ui import build_dagster_ui_steps, skip_if_no_dagster_ui_changes
from dagster_buildkite.steps.dagster_ui import (
build_dagster_ui_components_steps,
build_dagster_ui_core_steps,
skip_if_no_dagster_ui_changes,
)
from dagster_buildkite.steps.docs import build_docs_steps
from dagster_buildkite.steps.trigger import build_trigger_step
from dagster_buildkite.utils import BuildkiteStep, is_release_branch, safe_getenv
Expand Down Expand Up @@ -52,7 +56,8 @@ def build_dagster_oss_main_steps() -> List[BuildkiteStep]:
# Full pipeline.
steps += build_repo_wide_steps()
steps += build_docs_steps()
steps += build_dagster_ui_steps()
steps += build_dagster_ui_components_steps()
steps += build_dagster_ui_core_steps()
steps += build_dagster_steps()

return steps
Expand Down
39 changes: 35 additions & 4 deletions .buildkite/dagster-buildkite/dagster_buildkite/steps/dagster_ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,34 @@
from ..utils import CommandStep, is_feature_branch


def skip_if_no_dagster_ui_changes():
def skip_if_no_dagster_ui_components_changes():
if not is_feature_branch():
return None

# If anything changes in the ui-components directory
if any(
Path("js_modules/dagster-ui/packages/ui-components") in path.parents
for path in ChangedFiles.all
):
return None

return "No changes that affect the ui-components JS library"


def build_dagster_ui_components_steps() -> List[CommandStep]:
return [
CommandStepBuilder(":typescript: dagster-ui-components")
.run(
"cd js_modules/dagster-ui/packages/ui-components",
f"tox -vv -e {AvailablePythonVersion.to_tox_factor(AvailablePythonVersion.get_default())}",
)
.on_test_image(AvailablePythonVersion.get_default())
.with_skip(skip_if_no_dagster_ui_components_changes())
.build(),
]


def skip_if_no_dagster_ui_core_changes():
if not is_feature_branch():
return None

Expand All @@ -25,14 +52,18 @@ def skip_if_no_dagster_ui_changes():
return "No changes that affect the JS webapp"


def build_dagster_ui_steps() -> List[CommandStep]:
def build_dagster_ui_core_steps() -> List[CommandStep]:
return [
CommandStepBuilder(":typescript: dagster-ui")
CommandStepBuilder(":typescript: dagster-ui-core")
.run(
"cd js_modules/dagster-ui",
f"tox -vv -e {AvailablePythonVersion.to_tox_factor(AvailablePythonVersion.get_default())}",
)
.on_test_image(AvailablePythonVersion.get_default())
.with_skip(skip_if_no_dagster_ui_changes())
.with_skip(skip_if_no_dagster_ui_core_changes())
.build(),
]


def skip_if_no_dagster_ui_changes():
return skip_if_no_dagster_ui_components_changes() or skip_if_no_dagster_ui_core_changes()
1 change: 1 addition & 0 deletions js_modules/dagster-ui/packages/app-oss/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"start": "next dev",
"build": "next build",
"lint": "eslint src/ --ext=.tsx,.ts,.js --fix -c .eslintrc.js",
"lint:ci": "eslint src/ --ext=.tsx,.ts,.js -c .eslintrc.js",
"test": "jest",
"ts": "tsc -p .",
"analyze": "webpack-bundle-analyzer '.next/webpack-stats.json'"
Expand Down
1 change: 1 addition & 0 deletions js_modules/dagster-ui/packages/ui-components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"prepublish": "yarn lint && yarn ts && yarn jest",
"build": "rm -rf lib && tsc -p ./tsconfig.build.json && yarn rollup -c rollup.config.js",
"lint": "eslint src/ --ext=.tsx,.ts,.js --fix -c .eslintrc.js",
"lint:ci": "eslint src/ --ext=.tsx,.ts,.js -c .eslintrc.js",
"jest": "jest",
"jest-all-silent": "yarn jest --silent --watchAll=false",
"ts": "tsc -p .",
Expand Down
21 changes: 21 additions & 0 deletions js_modules/dagster-ui/packages/ui-components/tox.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
[tox]
skipsdist = True

[testenv]
download = True
passenv = CI_* COVERALLS_REPO_TOKEN AWS_SECRET_ACCESS_KEY AWS_ACCESS_KEY_ID BUILDKITE*
setenv =
STRICT_GRPC_SERVER_PROCESS_WAIT = "1"
usedevelop = False
allowlist_externals =
/bin/bash
git
yarn
commands =
!windows: /bin/bash -c '! pip list --exclude-editable | grep -e dagster'
yarn install
yarn ts
yarn lint:ci
yarn jest --clearCache
yarn jest-all-silent --testTimeout=10000 --ci --logHeapUsage --workerIdleMemoryLimit=1GB
git diff --exit-code
3 changes: 1 addition & 2 deletions js_modules/dagster-ui/packages/ui-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
"jest-all-silent": "yarn jest --silent --watchAll=false",
"jest:debug": "node --inspect-brk ./node_modules/.bin/jest --runInBand --no-cache",
"lint": "eslint src/ --ext=.tsx,.ts,.js,.graphql --fix -c .eslintrc.js",
"check-lint": "eslint src/ --ext=.tsx,.ts,.js,.graphql -c .eslintrc.js",
"check-prettier": "prettier --list-different \"./src/**/*.tsx\"",
"lint:ci": "eslint src/ --ext=.tsx,.ts,.js,.graphql -c .eslintrc.js",
"ts": "tsc -p .",
"generate-graphql": "ts-node -O '{\"module\": \"commonjs\"}' ./src/scripts/generateGraphQLTypes.ts",
"generate-perms": "ts-node -O '{\"module\": \"commonjs\"}' ./src/scripts/generatePermissions.ts",
Expand Down
14 changes: 4 additions & 10 deletions js_modules/dagster-ui/tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,10 @@ commands =
yarn install
yarn workspace @dagster-io/ui-core generate-graphql
yarn workspace @dagster-io/ui-core generate-perms
yarn workspace @dagster-io/app-oss ts
yarn workspace @dagster-io/app-oss lint
yarn workspace @dagster-io/ui-core ts
yarn workspace @dagster-io/ui-core lint
yarn workspace @dagster-io/ui-components ts
yarn workspace @dagster-io/ui-components lint
yarn workspace @dagster-io/ui-core lint:ci
yarn workspace @dagster-io/ui-core jest --clearCache
yarn workspace @dagster-io/ui-core jest-all-silent --testTimeout=10000 --ci --logHeapUsage --workerIdleMemoryLimit=1GB
yarn workspace @dagster-io/ui-core check-prettier
yarn workspace @dagster-io/ui-core check-lint
yarn workspace @dagster-io/ui-components jest --clearCache
yarn workspace @dagster-io/ui-components jest-all-silent --testTimeout=10000 --ci --logHeapUsage --workerIdleMemoryLimit=1GB
git diff --exit-code
yarn workspace @dagster-io/app-oss ts
yarn workspace @dagster-io/app-oss lint:ci
git diff --exit-code

2 comments on commit bfde747

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-pipffv116-elementl.vercel.app

Built with commit bfde747.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-ofsf767gp-elementl.vercel.app

Built with commit bfde747.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.