Skip to content

chore: consolidate the build #52

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.

Sign up for GitHub

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

Merged
junglebarry merged 1 commit into from
Jun 26, 2025
Merged

chore: consolidate the build #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ commands:
paths:
- node_modules
key: << pipeline.parameters.node_modules_cache_key >>

Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

Broken window: spacing.

save_cache_build:
steps:
- save_cache:
Expand All @@ -45,13 +45,16 @@ commands:
- build--{{ .Revision }}

jobs:
start:
start_build:
type: no-op
Comment on lines +48 to +49
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 26, 2025

Choose a reason for hiding this comment

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

Changed to no-op for efficiency.


complete_build:
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 26, 2025

Choose a reason for hiding this comment

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

This cannot be no-op because CircleCI does not report statuses to Github status checks for no-ops. We add branch protection against this job.

<<: *executor_node
steps:
- run:
name: Start
command: |
echo Start
name: "Complete build"
command: echo "Build completed successfully"

lint_and_test:
<<: *executor_node
steps:
Expand All @@ -64,14 +67,17 @@ jobs:
- run:
name: "Test"
command: npm run test
- run:
name: "Build"
command: npm run build
Comment on lines +70 to +72
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

We also want to know that the docs build correctly.

Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 26, 2025

Choose a reason for hiding this comment

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

I am a bit puzzled at this point about what should and should not be built, but we want to ensure it doesn't fail in branch builds before merging into main. As follow-ups, I'll work towards either:

  1. Checking that any generated artifacts that must be under version control are indeed updated (i.e. a porcelain check)
  2. Excluding any truly generated artifacts from version control, if possible.

I can't really tell which it needs to be, yet, but will see once a few builds go through.

Copy link
Copy Markdown
Contributor

@twba-a twba-a Jun 26, 2025

Choose a reason for hiding this comment

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

So from what I understand currently the build files are committed and nothing is actually in gitignore . not sure why it was done this way, it could be because its using eleventry and its hosted in github-pages ! so to keep the deployment simple we might want to keep as is but maybe we should be ok to ignore .map files as they are just debug artifacts!
For the check, I wonder if we can have another step in circleCI to do git diff between what came out after the build to whats been committed in the code source!

Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 26, 2025

Choose a reason for hiding this comment

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

So from what I understand currently the build files are committed and nothing is actually in gitignore . not sure why it was done this way, it could be because its using eleventry and its hosted in github-pages ! so to keep the deployment simple we might want to keep as is but maybe we should be ok to ignore .map files as they are just debug artifacts! For the check, I wonder if we can have another step in circleCI to do git diff between what came out after the build to whats been committed in the code source!

Yes - that's the first option - a porcelain check.

push_to_github_pages:
<<: *executor_node
steps:
- checkout
- restore_cache_node_modules
- run:
name: Generate documentation
command: npm run build:eleventy
command: npm run build
Comment on lines -74 to +80
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should have been building the CSS as well as the eleventy files - build does both.

- run:
name: Configure dependencies
command: |
Expand All @@ -84,25 +90,25 @@ jobs:
workflows:
build-and-test:
jobs:
- start:
name: Start
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

The names just confuse matters, which is why the build hasn't been running. Removed, accordingly.

Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

... nope. The names confused me; but the build hasn't been running because the branch protection doesn't make it obvious they need to. In any case, I've aligned these with conventions elsewhere.

- start_build
- be_kind_to_your_colleagues:
name: Be kind to your colleagues
type: approval
filters:
branches:
ignore:
- main
- lint_and_test:
name: Lint and test
requires:
- Start
- Be kind to your colleagues
- start_build
- be_kind_to_your_colleagues
Comment on lines -99 to +103
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

I think these names were the reason the build never ran.

- push_to_github_pages:
name: Push to Github Pages
requires:
- Lint and test
- lint_and_test
filters:
branches:
only:
- main
- complete_build:
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

A fan-in step, to simplify the branch protection statuses

Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 26, 2025

Choose a reason for hiding this comment

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

... note that this is not a no-op because those do not report statuses back to Github, and we need to use the status report in the branch protection rules.

requires:
- lint_and_test
- push_to_github_pages
1 change: 1 addition & 0 deletions .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
save-exact=true
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Broken window: We went through an exercise of pinning the dependency versions - this keeps it that way.

@talis:registry=https://npm.pkg.github.com
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
18.14
18.14.0
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Broken window: Aligned explicitly with the version used in the CircleCI image tag.

6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"homepage": "https://github.com/talis/bootstrap-theme#readme",
"scripts": {
"build": "npm-run-all build:*",
"build:css": "sass scss/:docs/assets/css/ --style=compressed --load-path=node_modules --source-map",
"build:css": "npm run css-compile",
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Broken window: The definitions are the same, so compose rather than duplicating.

"build:eleventy": "eleventy",
"css": "npm-run-all css-compile css-prefix",
"css-compile": "sass scss/:docs/assets/css/ --style=compressed --load-path=node_modules --source-map",
Expand All @@ -33,10 +33,10 @@
"release": "standard-version",
"server": "serve docs --listen 3000",
"start": "npm-run-all --parallel build:css watch:*",
"test": "npm run css-lint && npm run css",
"test": "npm-run-all css-lint css",
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025

Choose a reason for hiding this comment

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

Broken window: use the tools.

"watch": "nodemon -e html,scss -x \"npm run css\"",
"watch:eleventy": "eleventy --serve",
"watch:sass": "sass scss/:docs/assets/css/ --style=compressed --load-path=node_modules --source-map --watch"
"watch:sass": "npm run css-compile -- --watch"
Copy link
Copy Markdown
Contributor Author

@junglebarry junglebarry Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Broken window: Again, except for --watch this is the same as css-compile - use it via composition instead.

},
"peerDependencies": {
"bootstrap": "5.2.2",
Expand Down