-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Creating the NewHelp site with a default Jekyll installation #49628
Conversation
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
A preview of your New Help changes have been deployed to https://4e7bad84.newhelp.pages.dev ⚡️ |
Hey @DylanDylann I'm reassigning this to @coleaeason as he's the one who has written up the instructions I'm following now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! nab b/c we're going to delete the default pages, but could change .markdown
to .md
to match our normal conventions on file naming.
.github/workflows/deployNewHelp.yml
Outdated
|
||
- name: Purge Cloudflare cache | ||
if: env.IS_PR_FROM_FORK != 'true' | ||
run: $HOME/.local/bin/cli4 --verbose --delete hosts=["help.expensify.com"] /zones/:9ee042e6cfc7fd45e74aa7d2f78d617b/purge_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh the linter is complaining about this line. It wants you to put double quotes around $HOME/.local/bin/cli4
to prevent word splittding on $HOME
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
.github/workflows/deployNewHelp.yml
Outdated
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove fetch-depth: 0
. We really only need the snapshot of the repo with the latest commit to run a build, not the full history of all branches and tags. Removing this will save us more than a minute on each workflow run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump on this change. It makes the workflow much faster and is documented as a best practice here: https://github.com/Expensify/App/blob/44d09e58e73e937ad60e1c26107bfc022f5e4674/.github/workflows/README.md#fast-fetch
.github/workflows/deployNewHelp.yml
Outdated
with: | ||
ruby-version: '3.3.4' | ||
|
||
- name: Install dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify things and make them a bit more efficient by letting ruby/setup-ruby
run the bundle install for us:
-
Add a
.ruby-version
file tohelp/
(or symlink the one at the repo root tohelp/.ruby-version
, not 100% sure if this action will follow symlinks) -
Then adjust the
setup-ruby
step like so:- name: Setup Ruby and install gems uses: ruby/setup-ruby@v1 with: bundler-cache: true working-directory: ./help
-
Remove the
Install dependencies
step
webrick (1.8.2) | ||
|
||
PLATFORMS | ||
aarch64-linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why so many platforms here ... none of our other Gemfile.locks
have so many platforms. Not sure if it matters though 🤷🏼
Nice! I got it working!!! Thanks for your help @coleaeason and @roryabraham , taking off HOLD. Can you please approve again? |
Hm, but it's not passing this validate test:
I wrapped the whole thing in double quotes, I'm not sure what else to do there. Any ideas? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think normally you could fix that ShellCheck error by just wrapping "$HOME"
in quotes, but it looks like there's a bug with actionslint that makes it 🤮 if you try to do that. So we can fix this just by using the full path for the runner home, /home/runner/
, like we do in other actions.
A few other things:
- Let's give the workflow a name
- we should simplify things and allow some more efficient caching by letting setup-ruby run the bundle install
- Let's add a
./help/.ruby-version
file to keep people's local version of ruby in sync with the version used indeployNewHelp
and consolidate the source of truth for ruby version - Let's remove
fetch-depth: 0
from the checkout. We don't need or want it - there are a few other actions that are skipped for changes only in the
/docs
directory. i.e: we don't need to run app performance regression tests on a HelpDot PR. We should apply that change with the/help
directory as well. - We can run the pip install and cloudflare commands from root, the working dir doesn't matter for those.
Here's a full diff of the changes I'd like us to make before we merge this:
Full diff
diff --git a/.github/workflows/deployNewHelp.yml b/.github/workflows/deployNewHelp.yml
index 5262e246842..917ce1b3c0e 100644
--- a/.github/workflows/deployNewHelp.yml
+++ b/.github/workflows/deployNewHelp.yml
@@ -1,3 +1,5 @@
+name: Deploy New Help Site
+
on:
# Run on any push to main that has changes to the help directory
push:
@@ -30,25 +32,16 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v4
- with:
- fetch-depth: 0
- # Set up Ruby and run commands inside the /help directory
+ # Set up Ruby and run bundle install inside the /help directory
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
- ruby-version: '3.3.4' # Match your local Ruby version
bundler-cache: true
-
- - name: Install dependencies
- run: |
- bundle config set --local path 'vendor/bundle'
- bundle install # Install all gems, including custom plugins
- working-directory: ./help # Make sure dependencies are installed inside /help
+ working-directory: ./help
- name: Build Jekyll site
- run: |
- bundle exec jekyll build --source ./ --destination ./_site # Build within /help
+ run: bundle exec jekyll build --source ./ --destination ./_site
working-directory: ./help # Ensure Jekyll is building the site in /help
- name: Deploy to Cloudflare Pages
@@ -64,20 +57,17 @@ jobs:
- name: Setup Cloudflare CLI
if: env.IS_PR_FROM_FORK != 'true'
run: pip3 install cloudflare==2.19.0
- working-directory: ./help # Run CLI setup in /help
- name: Purge Cloudflare cache
if: env.IS_PR_FROM_FORK != 'true'
- run: "$HOME/.local/bin/cli4 --verbose --delete 'hosts=[\"newhelp.expensify.com\"]' /zones/:9ee042e6cfc7fd45e74aa7d2f78d617b/purge_cache"
-
+ run: /home/runner/.local/bin/cli4 --verbose --delete hosts=["newhelp.expensify.com"] /zones/:9ee042e6cfc7fd45e74aa7d2f78d617b/purge_cache
env:
CF_API_KEY: ${{ secrets.CLOUDFLARE_TOKEN }}
- working-directory: ./help # Ensure cache purging is executed in /help
- name: Leave a comment on the PR
uses: actions-cool/maintain-one-comment@v3.2.0
if: ${{ github.event_name == 'pull_request' && env.IS_PR_FROM_FORK != 'true' }}
with:
- token: ${{ secrets.OS_BOTIFY_TOKEN }}
+ token: ${{ github.token }}
body: ${{ format('A preview of your New Help changes have been deployed to {0} :zap:️', steps.deploy.outputs.alias) }}
diff --git a/.github/workflows/preDeploy.yml b/.github/workflows/preDeploy.yml
index 79646817027..bfe860e6022 100644
--- a/.github/workflows/preDeploy.yml
+++ b/.github/workflows/preDeploy.yml
@@ -4,7 +4,7 @@ name: Process new code merged to main
on:
push:
branches: [main]
- paths-ignore: [docs/**, contributingGuides/**, jest/**, tests/**]
+ paths-ignore: [docs/**, help/**, contributingGuides/**, jest/**, tests/**]
jobs:
typecheck:
diff --git a/.github/workflows/reassurePerformanceTests.yml b/.github/workflows/reassurePerformanceTests.yml
index d4a25a63952..fb7a34d6fa0 100644
--- a/.github/workflows/reassurePerformanceTests.yml
+++ b/.github/workflows/reassurePerformanceTests.yml
@@ -4,7 +4,7 @@ on:
pull_request:
types: [opened, synchronize]
branches-ignore: [staging, production]
- paths-ignore: [docs/**, .github/**, contributingGuides/**, tests/**, '**.md', '**.sh']
+ paths-ignore: [docs/**, help/**, .github/**, contributingGuides/**, tests/**, '**.md', '**.sh']
jobs:
perf-tests:
diff --git a/.github/workflows/sendReassurePerfData.yml b/.github/workflows/sendReassurePerfData.yml
index 42d946cece9..884182bfc89 100644
--- a/.github/workflows/sendReassurePerfData.yml
+++ b/.github/workflows/sendReassurePerfData.yml
@@ -3,7 +3,7 @@ name: Send Reassure Performance Tests to Graphite
on:
push:
branches: [main]
- paths-ignore: [docs/**, contributingGuides/**, jest/**]
+ paths-ignore: [docs/**, help/**, contributingGuides/**, jest/**]
jobs:
perf-tests:
diff --git a/help/.ruby-version b/help/.ruby-version
new file mode 100644
index 00000000000..a0891f563f3
--- /dev/null
+++ b/help/.ruby-version
@@ -0,0 +1 @@
+3.3.4
.github/workflows/deployNewHelp.yml
Outdated
uses: actions-cool/maintain-one-comment@v3.2.0 | ||
if: ${{ github.event_name == 'pull_request' && env.IS_PR_FROM_FORK != 'true' }} | ||
with: | ||
token: ${{ secrets.OS_BOTIFY_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB but generally it's a best practice to just use the github.token
unless you have a reason it needs to be OSBotify
. The built-in token has higher rate limits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving because the push trigger is there on the existing deployExpensifyHelp.yml
workflow, so NAB I suppose. But I still don't really get why it's there 😄
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.0.42-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.42-3 🚀
|
Details
Implements step 2.1 of New.Help.Expensify.com
Fixed Issues
$https://github.com/Expensify/Expensify/issues/414789
Tests
Offline tests
n/a
QA Steps
n/a
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos