Skip to content

Tweak npm scripts #1186

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Tweak npm scripts #1186

wants to merge 1 commit into from

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Mar 2, 2025 •
edited
Loading

Now CI runs build once.

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions
  3. Sign all your commits as they must have verified signatures
  4. File a pull request for any change that requires changes to our documentation at our documentation repo

What does this PR aim to accomplish?:

How does this PR accomplish the above?:

Link documentation PRs if any are needed to support this PR:

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.
  • I have read the above and my PR is ready for review. Check this box to confirm

@XhmikosR XhmikosR marked this pull request as ready for review March 2, 2025 07:23
@netlify Netlify
Copy link

netlify bot commented Mar 2, 2025 •
edited
Loading

✅ Deploy Preview for pihole-docs ready!

Name Link
🔨 Latest commit e7cf217
🔍 Latest deploy log https://app.netlify.com/sites/pihole-docs/deploys/67c5fe351139ec0008c00202
😎 Deploy Preview https://deploy-preview-1186--pihole-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

XhmikosR
XhmikosR commented Mar 3, 2025
View reviewed changes
package.json Outdated
@@ -17,8 +17,9 @@
"homepage": "https://docs.pi-hole.net/",
"scripts": {
"build": "mkdocs build --clean",
"build:strict": "npm run build -- --strict",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also just move --strict in the build script and remove the new one.

@XhmikosR XhmikosR force-pushed the xmr/npm-scripts branch 2 times, most recently from 752dc88 to b3438e7 Compare March 3, 2025 16:28
@yubiuser
Copy link
Member

yubiuser commented Mar 3, 2025

I see you force pushing today twice - is this PR ready for review?

@XhmikosR
Tweak npm scripts
e7cf217 
Now CI runs build once.

Signed-off-by: XhmikosR <[email protected]>
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 3, 2025

Now it should be :)

I was in the middle about using the npm script in CI, but better keep things simple and organized in npm scripts. I also added a npm start script to start the server as a shortcut.

yubiuser
yubiuser reviewed Mar 3, 2025
View reviewed changes
package.json
"test": "npm run markdownlint && npm run linkinator",
"serve": "mkdocs serve --dev-addr 0.0.0.0:8000"
"serve": "mkdocs serve --dev-addr 0.0.0.0:8000",
"start": "npm run serve"
Copy link
Member

Choose a reason for hiding this comment

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

I also added a npm start script to start the server as a shortcut.

I'm a bit undecided about this. There is not much difference in npm run start vs npm run serve. Or is npm run start something 'common' one should have (best practice?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is that one can do npm start and it works, while serve needs npm run serve. So, it's better to follow the standard here and make everyone's lives easier :)

@XhmikosR XhmikosR closed this Mar 4, 2025
@XhmikosR XhmikosR deleted the xmr/npm-scripts branch March 4, 2025 05:24
@XhmikosR XhmikosR restored the xmr/npm-scripts branch March 4, 2025 05:24
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 4, 2025

Accidentally deleted the branch and I cannot reopen it seems.

XhmikosR
XhmikosR commented Mar 4, 2025
View reviewed changes
.github/workflows/ci.yml
@@ -41,7 +41,7 @@ jobs:
run: npm ci

- name: Build docs
run: mkdocs build --strict
run: npm run build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I'd like to keep this the same as Netlify. So, let's wait after #1194 is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yubiuser yubiuser yubiuser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@XhmikosR @yubiuser