-
Notifications
You must be signed in to change notification settings - Fork 108
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
GH-325: [Website] Update website deployment script to use latest LTS version of Node.js and Webpack 5.75.0 #326
Conversation
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
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.
Thanks @kevingurney for the PR!
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.
LGTM!
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.
We use Node.js 12 on Ubuntu 22.04 with this change, right?
https://github.com/apache/arrow-site/actions/runs/4314961494/jobs/7528685953#step:13:17
npm WARN EBADENGINE current: { node: 'v12.22.9', npm: '8.5.1' }
Node.js 12 is an EOL-ed Node.js. So it looks like that this is similar to 1. in GH-325.
It seems that 2. in GH-325 can be implemented with the following:
$ npm install webpack # this updates package.json and package-lock.json
If it works, it seems that it's a straitforword approach.
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
…e package list. Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
.github/workflows/deploy.yml
Outdated
@@ -27,17 +27,31 @@ jobs: | |||
deploy: | |||
name: Deploy | |||
runs-on: ubuntu-latest | |||
container: | |||
image: ubuntu:22.04 # Ubuntu 22.04 is the latest LTS version as of April 21, 2022: https://wiki.ubuntu.com/Releases |
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.
Can we try reverting this and changes related to this? (e.g.: explicit apt-get
, ImageOS: ubuntu22
and so on)
If we can use the ubuntu-latest
image directly, we can use simpler deploy.yml
and don't need to maintain Ubuntu version for 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.
I'm OK with just using ubuntu-latest
and no container. However, I know there were some concerns about this approach expressed by @avantgardnerio (e.g. the proprietary nature and lack of debuggability of ubuntu-latest
).
However, I can try this approach and if no one else in the community has reservations about it, then I am OK with 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 testing out the workflow with no container now on https://github.com/mathworks/arrow-site/tree/GH-325-no-container.
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.
Ah, I can understand the debuggability concern. If we make this job local debuggable, we should not use actions/*
in ubuntu:XXX
container like we did in apache/arrow: https://github.com/apache/arrow/blob/main/.github/workflows/cpp.yml#L92-L99
If we need local debuggability, we may need to choose another approach.
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.
Thanks.
It seems to work. I prefer the with container approach to the no container approach but I want to hear opinions from others.
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.
Thanks for confirming this.
How about the following steps to improve the current situation?
- We merge "no container" version https://github.com/mathworks/arrow-site/tree/GH-325-no-container because it's simpler than this version
- We create a
Dockerfile
based onubuntu:22.04
and use it in CI and https://github.com/apache/arrow-site#using-docker (This prevents breaking @alamb 's use case accidentally) - We extract command lines used in 2. as a shell script and fine tune the shell script to work on host Ubuntu 22.04 too (@avantgardnerio 's use case)
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.
Thanks @avantgardnerio and @kou for your sharing your thoughts! I agree. This sounds like a reasonable path forward.
Just to clarify:
"We create a Dockerfile based on ubuntu:22.04 and use it in CI"
By "use it in CI" do you mean that you would like the GitHub Actions workflow to run inside of a Docker container based on this Dockerfile
? If so, doesn't this conflict with 1. (which is to use the "no container" workflow)?
Sorry for the confusion. I just want to make sure we are all on the same page before moving forward.
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.
My idea is that we just use docker build && docker run
in GitHub Actions instead of container:
.
(More specifically, we will use docker/build-push-action
instead of raw docker build
and push a built image to ghcr.io
to reuse the built image on local for https://github.com/apache/arrow-site#using-docker . https://github.com/groonga/groonga-delta/blob/main/.github/workflows/docker.yml is a bit old but may help you to understand my idea.)
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.
If my explanation isn't enough, please ask me more!
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.
Thanks for clarifying @kou! This make sense!
I'll start working on these changes now.
Are we happy with this PR at the moment? Any concerns about merging it ? |
Ah, sorry. I forgot to left a comment. I'll left a comment soon. |
I'm happy... I'll wait a little bit for @kou |
Commented: #326 (comment) |
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.
How about revert ci/
changes for now and merge this pull request to fix the current CI failure?
We can work on the step 2. with ci/
changes as a follow-up pull request.
Sounds good to me. I'll revert the I've created a follow up issue #331 to capture the desire to improve the container workflow. |
I've reverted the changes and updated the PR title and description to more accurately reflect the changes. |
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.
+1
Thanks!
Overview
This pull request modifies the
apache/arrow-site
website deployment workflow (.github/workflows/deploy.yml
) to use the latest LTS version of Node.js and Webpack 5.75.0 to work around the build issue described in #325.Qualification
To qualify these changes, I:
main
branch of themathworks/arrow-site
fork in order to trigger thegh-pages
deployment workflow. I then selectedgh-pages
as the GitHub Pages deployment branch and verified that the site was deployed as expected to https://mathworks.github.io/arrow-site/. For an example of a successful workflow run, see: https://github.com/mathworks/arrow-site/actions/runs/4313253336/jobs/7524824999.Future Directions
asf-site
togh-pages
in the "Pages" settings of themathworks/arrow-site
fork. This wasn't immediately obvious, and it isn't listed explicitly as a required step in the README.md ofapache/arrow-site
. It would helpful to add an explicit note about this step. I've captured this as [Website] Add note about the need to set GitHub Pages deployment source branch togh-pages
when previewing website changes on a fork ofapache/arrow-site
#327 and addressed it with PR GH-327: [Website] Add note about the need to set GitHub Pages deployment source branch togh-pages
when previewing website changes on a fork ofapache/arrow-site
#328.deploy.yml
) is failing due to Node.js 18 version bump inubuntu-latest
GitHub Actions runner image and Webpack usage ofmd4
hashing algorithm #325, there is still more we could choose to do to address the root cause of these build failures (the deprecation of themd4
hash algorithm in Node 18). This would include setting theoutput.hashFunction
toxxhash64
for Webpack.Notes
ubuntu:latest
container!Closes #325.