-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add a workflow to build docker image #202
Conversation
0014a3b
to
953ac35
Compare
@@ -41,6 +41,7 @@ module.exports = { | |||
}, | |||
resolve: { | |||
extensions: ['.ts', '.js'], | |||
fallback: { "querystring": require.resolve("querystring-es3") } |
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.
Apparently querystring has been deprecated for awhile? It was causing errors failing to resolve and this is the answer my internet googling turned up.
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 the proper fix is to use URLSearchParams
where you can, as it's native to the browser. Not worth fixing in this PR but we should tidy up our frontend code.
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'll look at fixing it in a separate PR, thanks for giving the correct fix!
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.
(hopefully) fixed in #205
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: 18 | ||
# TODO build:web fails currently, but it'd be good to make that work when possible and then add it here. | ||
- run: yarn |
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.
This is just a drive-by cleanup now that build:web
is sorted out.
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 moving this to a separate PR so that building the docker images is not stuck behind getting access to the repo controls and changing the required checks. The changes have been moved to here: #204
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.
This looks good 👍 . Just one thought on using ghcr over dockerhub.
@@ -17,7 +17,7 @@ limitations under the License. | |||
import { Response, Request } from "express"; | |||
import template from "./utils/template" | |||
import config from "./config"; | |||
import { base32 } from "rfc4648"; | |||
const { base32 } = require('rfc4648'); |
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 don't pretend to understand javascript modules but I hit a compilation error here after merging in main and this fixed it.
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 going to quickly check this one. Technically this drops types so we might introduce an error if the function is undefined
or something.
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.
Two small changes then I think we're good to go.
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 rock 🤘
Fixes #200.