-
Notifications
You must be signed in to change notification settings - Fork 90
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
Deploy catalog from CI #3872
Deploy catalog from CI #3872
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3872 +/- ##
=======================================
Coverage 36.23% 36.23%
=======================================
Files 716 716
Lines 31618 31618
Branches 4688 4688
=======================================
Hits 11457 11457
Misses 19005 19005
Partials 1156 1156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I'm intentionally pushing only to ECR, not S3. S3 is only used for RODA - and updating RODA is such a manual process that I don't think there's any benefit in slowing down CI for that. |
well, yes |
@sir-sigurd i'd vote for getting rid of this discrepancy and keeping a single deployment / serving approach for the sake of simplicity. i doubt it very much serving w/ nginx would be a perf bottleneck. maybe we would save a couple $ tho, idk. @akarve wdyt? |
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.
shouldn't it depend on the tests passing?
Well, that's why I wanted to split everything up in #3866 - but that looks more complicated than I expected. But in any case, if we already require tests to pass before merging into |
d7139e2
to
302f222
Compare
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: '16.11' | ||
cache: 'npm' |
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.
But does this do anything by itself, without the cache-dependency-path
? From the docs, it sounds like it will only look for package-lock.json
in the root directory, and not in catalog/package-lock.json
.
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.
But does this do anything by itself, without the cache-dependency-path?
i assumed that it would just never invalidate the cache, but that turned out wrong. i fixed it by bringing back catalog/package-lock.json
I see this is merged but I support at least trying to remove the CloudFront special casing for open b/c I agree it's complex. The CDN probably does help SEO scores (page load times) but how much I don't know. Worth measuring the perf difference, as I don't think we ever did. And even if nginx ends up slower maybe we can just scale up those container services. |
No description provided.