-
Notifications
You must be signed in to change notification settings - Fork 5
chore: include generated assets in repo #2201
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
chore: include generated assets in repo #2201
Conversation
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer:
Airgap Installer (may take a few minutes before the airgap bundle is built):
Happy debugging! |
.github/workflows/ci.yaml
Outdated
check-web: | ||
name: Check web | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
- name: Setup node | ||
uses: actions/setup-node@v4 | ||
with: | ||
node-version: lts/* | ||
- name: Install dependencies | ||
working-directory: ./web | ||
run: npm i | ||
- name: Build | ||
working-directory: ./web | ||
run: npm run build | ||
- name: Check web is up to date | ||
run: | | ||
git diff --exit-code --name-only | ||
if [ $? -eq 0 ]; then | ||
echo "Web is up to date" | ||
else | ||
echo "Web is out of date" | ||
exit 1 | ||
fi |
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.
Alternatively, why don't we build the web assets on CI VS having to locally build it and commit it? Things such as npm install
are non-deterministic, meaning my local web build can be different from yours. In CI we can make sure we have reproducible builds via specific node+npm versions and commands such as npm ci
for example.
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.
Overall LGTM, have one suggestion and one question
web/vite.config.ts
Outdated
plugins: [ | ||
react(), | ||
// Preserve README.md from .gitignore | ||
viteStaticCopy({ | ||
targets: [ | ||
{ | ||
src: path.resolve(__dirname, './README.md'), | ||
dest: './', | ||
}, | ||
], | ||
}), | ||
], |
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.
Could you explain why we need this?
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.
updated the comment
Co-authored-by: João Antunes <[email protected]>
Test failures are unrelated. Merging |
What this PR does / why we need it:
Adds generated assets to repo and checks that they are up to date or fails the CI run.
Which issue(s) this PR fixes:
Does this PR require a test?
Does this PR require a release note?
Does this PR require documentation?