HADOOP-19858. Set up build workflow on GitHub Actions#8412
HADOOP-19858. Set up build workflow on GitHub Actions#8412pan3793 merged 1 commit intoapache:trunkfrom
Conversation
| uses: docker/build-push-action@d08e5c354a6adb9ed34480a06d141179aa583294 # v7.0.0 | ||
| with: | ||
| context: ./dev-support/docker/ | ||
| file: ./dev-support/docker/Dockerfile_${{ inputs.os }} |
There was a problem hiding this comment.
fresh building takes 15~20 min, will speed it up by using cache, in the next PR.
| uid: ${{ steps.variables.outputs.uid }} | ||
| steps: | ||
| - name: Login to GitHub Container Registry | ||
| uses: docker/login-action@b45d80f862d83dbcd57f89517bcf500b2ab88fb2 # v4.0.0 |
There was a problem hiding this comment.
follow ASF GHA policy, - MUST pin all external actions (other than apache/*, github/* and actions/*) to the specific git hash (SHA1)
|
@ajfabbri @ayushtkn @slfan1989, what do you think of this direction? if it works well, we can at least remove the native build jobs from the Jenkins pipeline. |
|
🎊 +1 overall
This message was automatically generated. |
|
I am pretty much in favour of delegating as much as possible to github actions and maybe just let tests run as part of the Jenkins CI. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ajfabbri
left a comment
ajfabbri
left a comment
There was a problem hiding this comment.
Thanks for working on this! Looks pretty good, but I have a couple of questions.
|
💔 -1 overall
This message was automatically generated. |
|
need to diagnose the container image write permission issue, which was not present in my test (because I'm the owner of the forked repo), convert to draft for now. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
@ajfabbri yes, but this is irrelevant to security, the intention here is to save resources, I remember in those days, the forked repos also auto-run cron jobs by default. - note, the contributor has full control over their forked repo, so they can remove spark has a lot of profiles (java/scala/python/arrow/pandas versions, ANSI/non-ANSI, sbt/maven, etc.), a full combination produces a large matrix, so it selects a part of that to run on PR and push, and schedules daily jobs (you can find those jobs status in the README.md) on
I'm not sure what your definition of "privileged". I think it's a normal use case of GHA, the same workflow can be triggered by different events from different contexts, we just need to be careful with one case - a workflow can be triggered on the upstream context, and the workflow consumes untrusted code from PR.
I don't think this is something we want to do, for unit tests. Generally, testing requires more dev dependencies, which might not be required by the runtime, for example, in Maven, we can define dependencies in compile, runtime, test scopes, when runs UT, it pulls the runtime + test scopes deps into the classpath, similar things apply to native libs. TBH, I didn't see many production use cases that deploy Hadoop in containers, and obviously, the current "pre-installed images" are mostly used for downstream projects for testing, and it only covers a few cases, at least kerberized YARN is likely not to work - for hadoop 3.4.x, official hadoop bin tgz was built on ubuntu 20.04, with openssl 1.x, while the pre-installed images is based on the ubuntu 22.04, which only has openssl 3.x in apt repo, IIRC it will fail the kerberized linux container to start. while building some smoking/integration tests like spark kubernetes/integration-tests based on the "pre-installed images" might be a good supplement in the future, but obviously, this is out of the scope of the current goals |
|
@steveloughran, I'm not the author of the Spark GHA workflow, but yes, I think we have the same understanding of that.
this repo has low traffic, I'm not sure if we can exercise it thoroughly. |
|
Thanks again for explaining. So you propose just building the "build" or "infra" images for now? I am fine with that. 👍 I think it would be cool to build the preinstalled containers with workflows too in the future, but not now. |
|
@ajfabbri yes, "build" or "infra" -only image (I borrowed "infra" from spark, because it has a lot of variants of images for different purposes - PySpark testing, SparkR testing, docs building, releasing, etc.), if this sounds misleading, we can call it "build", to match |
|
Either works for me 👍 I will try to spend some time tomorrow testing with your branch (now that I understand more) and see if we can get it ready for merging. 🤞 |
|
@ajfabbri thanks, let me tune the image name and have a rebase, as trunk has moved forward. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Next steps before we can merge this (edit: updated):
|
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
so CodeQL works? but no comments because it does not find negative issues? |
Yes, it looks good! No new warnings. Our existing actions have two. |
There was a problem hiding this comment.
Ok I'm ready to approve this. Do you think it is ready?
I updated my #8412 (comment) on remaining tasks. I propose we:
- Fix the rebase (CI is complaining about it). I can do this for you if you want, before merging.
- I will follow up on my question to INFRA https://issues.apache.org/jira/browse/INFRA-27839. It shouldn't affect us now, but I'd like to make sure we're protected if there are other
pull_request_targetworkflows added in the future. - I will add my security review comments in a separate PR (I am eager to add some stuff for S3A testing after this is merged!) (FWIW, updated here).
|
@pan3793 @ajfabbri @steveloughran Thank you very much for your contribution! +1. I plan to start next week and can also participate in some of the work. |
@ajfabbri, it's likely a false-negative check result by Yetus, sometimes retry can fix it but it seems that does not work this time, I might need to squash the commit to make it happy |
pan3793
force-pushed
the
HADOOP-19858
branch
2 times, most recently
from
c4cc606 to
ba9c52c
Compare
@ajfabbri I just cherry-pick it into here. I didn't do that because it contained TODO previously, and is resolved now. update: I reverted it, because I think it needs more discussion after a closer look. I will leave it to you to add them in a subsequent PR |
| # the upstream repo. | ||
| # - For `pull_request_target` (risky), the write permission is | ||
| # overridden by our repository's setting "Send write tokens to workflows | ||
| # from pull requests" which should be disabled. |
There was a problem hiding this comment.
oh wait, we still need this permission for workflows like "notify_test_workflow.yml", those workflows won't consume untrusted code, so they're safe to run
There was a problem hiding this comment.
Yes I mentioned that in my previous comment. I think it is ok here because (1) it is only write access for updating checks, (2) we are careful not to be careless with untrusted inputs, we don't checkout the fork, etc. (3) I have an issue open with INFRA to double-check our repositories defaults and security settings.
If the writing to checks fails due to restrictive repository settings, we can revisit it.
There was a problem hiding this comment.
I reverted the pull that includes this comment and restored the code to the snapshot that gets approval (with additional squash to make Yetus happy).
So, merge this PR as-is after Yetus is happy, and revise those comments later?
There was a problem hiding this comment.
+1 works for me. I'll send out a PR with the comments after we get this merged and we confirm everything is working as expected.
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks all, Yetus is green, merged to trunk |
|
Woo hoo. Thanks for working together on this. Follow-up PR here: #8450 |
6 participants
Description of PR
This PR sets up a "build-only"(including all Java, Native, and Web code) workflow for Hadoop on GitHub Actions, the basic framework is largely inspired by Apache Spark (apache/spark#32193) - run CI jobs on the forked repo, which has two major benefits:
apache/hadooprepo.what runs on the forked repo, on each push
dev-support/docker/Dockerfile_*what runs on the apache/hadoop repo
This is an attempt for https://lists.apache.org/thread/k40c2yzb7fc7nj9rm37ov4dd3vfov9j7 - shift some CI jobs from Jenkins to GHA, and run them in parallel, to speed up Hadoop CI.
For reviewers: the current workflow is inspired by Apache Spark, you may find that the workflow is written in a kind of over-engineering pattern, but this is necessary for future extension.
How was this patch tested?
I created another forked repo from my forked repo, and verified this in my forked repo.
take this PR pan3793#3 as an example
when jobs are running
when jobs are completed
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
No AI usage.