Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 9, 2023

This should not affect CI runs that have DANGER_RUN_CI_ON_HOST=1 set, for example .cirrus.yml.

However, when running CI locally with podman or docker, the container is stopped and thus deleted when all tests have passed. This feature was requested in #26843 (comment) and can help to reduce used disk space when running several CI tasks subsequently.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 9, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26766 (ci: Use clang-15 and IWYU v0.19 in "tidy" task by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot changed the title ci: Stop and remove CI container ci: Stop and remove CI container Jan 9, 2023
@DrahtBot DrahtBot added the Tests label Jan 9, 2023
@maflcko maflcko force-pushed the 2301-ci-container-stop- branch from fa789b9 to fa868d2 Compare January 9, 2023 09:23
@fanquake
Copy link
Member

fanquake commented Jan 9, 2023

Concept ACK

@maflcko maflcko force-pushed the 2301-ci-container-stop- branch from fa868d2 to fad7ba8 Compare January 9, 2023 15:17
@hebasto
Copy link
Member

hebasto commented Jan 9, 2023

Concept ACK.

@maflcko maflcko force-pushed the 2301-ci-container-stop- branch from fad7ba8 to fae3f90 Compare January 11, 2023 09:44
MarcoFalke added 2 commits January 11, 2023 10:49
DOCKER in names is confusingly used as synonym for "image", "container",
and "ci". Fix the confusion by picking the term that fits the context.

-BEGIN VERIFY SCRIPT-
 ren() { sed -i "s:$1:$2:g" $( git grep -l "$1" ) ; }

 ren DOCKER_PACKAGES CI_BASE_PACKAGES
 # This better reflects that they are the common base for all CI
 # containers.

 ren DOCKER_ID CI_CONTAINER_ID
 # This is according to the documentation of "--detach , -d: Run
 # container in background and print container ID".

 ren DOCKER_NAME_TAG CI_IMAGE_NAME_TAG
 # This avoids confusing with CONTAINER_NAME and clarifies that it is an
 # image.

 ren DOCKER_ADMIN CI_CONTAINER_CAP
 # This clarifies that it is a capability added to the container.

 ren DOCKER_CI_CMD_PREFIX CI_EXEC_CMD_PREFIX
 # This brings it in line with the CI_EXEC naming.

-END VERIFY SCRIPT-
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa0584e

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa0584e, tested on Ubuntu 22.04.

My only concerns are about CI_ "namespace" for variables, which has being actively used by CI providers. No conflicts with Cirrus CI although.

@maflcko
Copy link
Member Author

maflcko commented Jan 12, 2023

Happy to pick another name, if there are suggestions.

@hebasto
Copy link
Member

hebasto commented Jan 12, 2023

Happy to pick another name, if there are suggestions.

Maybe BITCOIN_...?

@maflcko
Copy link
Member Author

maflcko commented Jan 12, 2023

Will leave as-is for now. Can be changed in a follow-up, because merge conflicts in this area are rare.

@maflcko maflcko merged commit f4ef856 into bitcoin:master Jan 12, 2023
@maflcko maflcko deleted the 2301-ci-container-stop-💺 branch January 12, 2023 19:49
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 12, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants