Switch to manual docker #79
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks a lot from bringing this in. At a first look, this looks very solid. I will later do a proper review, then we can discuss what's the strategy for bringing this in. |
carlopi
approved these changes
Sep 20, 2024
There was a problem hiding this comment.
Thanks a lot!
This looks great, there are some improvements we can consider moving forward, but this does look solid, and I think it's worth having this merged in.
Question, should we do first this into a next branch, then testing, then into main branch, or directly into main and then testing?
carlopi
added a commit
to duckdb/community-extensions
that referenced
this pull request
Sep 21, 2024
FIxes after duckdb/extension-ci-tools#79 & co
carlopi
added a commit
to carlopi/duckdb
that referenced
this pull request
Nov 26, 2024
Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
Mytherin
added a commit
to duckdb/duckdb
that referenced
this pull request
Nov 27, 2024
…RE_NODE_VERSION only on main/feature (#14998) Idea for this PR is avoiding some noise in CI and the potential confusion to DuckDB contributors. For additional context: * Using `container:` implies all actions are run within a given containerized image * for security & maintenance reasons, actions are required to use recent tooling * eventually those tools (like node 20) will not be supported on images such as manylinux_2014 Solution is running the dockerized parts piece-by-piece, while interacting with actions at the `native` layer. This has been worked on in the duckdb/extension-ci-tools repository, via duckdb/extension-ci-tools#79 and subsequent PRs, now we need to port this to duckdb/duckdb. This PR only avoid the noise in CI, given those workflow will fail without fix, I don't see much value in running them at all. Proper fix to be rolled piece-by-piece, Python for example will be eventually solved via: #14987, where this part has then to be reverted.
carlopi
added a commit
to carlopi/duckdb
that referenced
this pull request
Nov 27, 2024
Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
carlopi
added a commit
to carlopi/duckdb
that referenced
this pull request
Nov 27, 2024
Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
carlopi
added a commit
to carlopi/duckdb
that referenced
this pull request
Nov 27, 2024
Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
carlopi
added a commit
to carlopi/duckdb
that referenced
this pull request
Nov 27, 2024
Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
Mytherin
added a commit
to duckdb/duckdb
that referenced
this pull request
Nov 27, 2024
…ons (#14987) Idea is instead of executing whole workflow job containerized, that means that actions will be run within the container, that in turn means it's hard to use older images since they will not have the required dependecies versions for the actions (in particular node 20). Solution is to containerize explicitly the relevant tests in the workflow, invoking actions outside of the container in the regular image that powers the workflow. Heavy lifting has been done by @samansmink in duckdb/extension-ci-tools#79 and connected PRs. This PR allows to avoid the need for ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION in Python CI.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSIONflag from the Linux CI.It achieves this by refactoring the linux jobs to manually invoke docker for the build and test steps effectively pulling out the github actions from the docker image. This has the following benefits:
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSIONflagAlso during development I integrated #63 because I needed it, that saved me some CI cycles so I can just merge this PR and close the otherone.
There is quite some duplication between the dockerfiles, but I did not find a clean way to solve this yet, but any suggestions (or follow up PRs) are welcome ofc.
After this PR it makes sense to upstream this into duckdb/duckdb somehow because there we can reuse these images in CI as well to make things more robust and get rid of
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSIONAnother follow up is to finally enable the vcpkg build cache, because that seems to be able to make quite a difference as well judging from how long vcpkg runs in the delta extension build