Refactor CI verification flow and integration cleanup#110
Refactor CI verification flow and integration cleanup#110
Conversation
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
| docker run \ | ||
| -u "$(id -u)":"$(id -g)" \ | ||
| -v "$PWD":/go/build \ | ||
| -v /etc/ssl/certs:/host-certs:ro \ |
There was a problem hiding this comment.
Needed because the golang image runs go build as the non-root UID (-u), which cannot read /etc/ssl/certs there; mounting host certs and setting SSL_CERT_* avoids TLS failures when modules download. (5cfb35e)
testsuite/ceph-s3-tests/run.sh
Outdated
| apt-get update && apt-get -yqq install --no-install-recommends git \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
| python3 -m pip install --no-cache-dir --upgrade pip tox |
There was a problem hiding this comment.
why can't we install tox through apt?
There was a problem hiding this comment.
Switched back to installing tox via apt and removed the pip install so it aligns with the base image package manager. (213d503)
Jenkinsfile
Outdated
| environment { | ||
| GOLANGCI_LINT_CONFIG = '/go/ci/.golangci.yml' | ||
| GOLANGCI_LINT_CONFIG_TESTSUITE = '/go/ci/.golangci.yml' | ||
| } | ||
| steps { | ||
| sh 'make -j verify' | ||
| } | ||
| post { | ||
| always { | ||
| archiveArtifacts artifacts: '.build/tests.json', allowEmptyArchive: true | ||
| archiveArtifacts artifacts: '.build/testsuite.json', allowEmptyArchive: true | ||
| junit allowEmptyResults: true, testResults: '.build/tests.xml' | ||
| junit allowEmptyResults: true, testResults: '.build/testsuite.xml' |
There was a problem hiding this comment.
we still want granular steps; that's why we had so many Makefile targets.
grouping was mostly for local development purposes.
There was a problem hiding this comment.
Restored the parallel Lint/Cross-Vet/Test/Testsuite stages and kept each stage invoking make verify-* handlers for granular reporting. (213d503)
|
|
||
| // install storj-up dependency | ||
| sh 'go install storj.io/storj-up@main' |
There was a problem hiding this comment.
do we not want to install it here? we can also make sure it gets installed within the install-dev-dependencies handler
There was a problem hiding this comment.
Re-added storj-up install in Jenkins checkout and also added it to install-dev-dependencies for local parity. (213d503)
Makefile
Outdated
|
|
||
| # golangci-lint: | ||
| go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.57.0 | ||
| go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest |
There was a problem hiding this comment.
let's make sure that this version is consistent with the one that storj/ci uses
There was a problem hiding this comment.
Aligned the install to golangci-lint v2.9.0 (matching storjlabs/ci:latest) using the v2 module path. (213d503)
| ifneq ($(wildcard $(GOLANGCI_LINT_CONFIG)),) | ||
| GOLANGCI_LINT_CONFIG_ARG := --config ${GOLANGCI_LINT_CONFIG} | ||
| endif | ||
| ifneq ($(wildcard $(GOLANGCI_LINT_CONFIG_TESTSUITE)),) | ||
| GOLANGCI_LINT_CONFIG_TESTSUITE_ARG := --config ${GOLANGCI_LINT_CONFIG_TESTSUITE} | ||
| endif |
There was a problem hiding this comment.
what is the meaningful aspect of this change?
There was a problem hiding this comment.
The conditional config args let golangci-lint run locally when ../ci/.golangci.yml is missing while still honoring it when present. (5cfb35e)
There was a problem hiding this comment.
ah, right—good catch. BTW let's make sure that you clone the storj/ci repo for your next run to ensure that the version upgrade works with the config file it's supposed to be used with.
There was a problem hiding this comment.
Cloned storj/ci into /tmp/storj-ci and ran make verify with GOLANGCI_LINT_CONFIG pointing at its .golangci.yml to validate the upgrade.
|
|
||
| BUILD_NUMBER ?= ${TAG} | ||
| INTEGRATION_COMPOSE_PROJECT ?= integration-${BUILD_NUMBER} | ||
| INTEGRATION_BUILD_DIR ?= .build |
There was a problem hiding this comment.
we can make the test script clean up this directory since it creates it
There was a problem hiding this comment.
Removed .build/s3-tests cleanup from the Makefile since run.sh now handles it via its trap. (213d503)
There was a problem hiding this comment.
should we make other scripts clean up after themselves as well?
There was a problem hiding this comment.
Left other scripts as-is since their outputs are still needed for Jenkins artifacts; cleanup remains centralized in integration-env-clean for those paths.
Makefile
Outdated
| ##@ Local development/Public Jenkins/Integration Test | ||
|
|
||
| BUILD_NUMBER ?= ${TAG} | ||
| INTEGRATION_COMPOSE_PROJECT ?= integration-${BUILD_NUMBER} |
There was a problem hiding this comment.
should there be a reasonable default for $BUILD_NUMBER?
when it's not run within Jenkins, it's going to be blank
There was a problem hiding this comment.
Added a fallback so BUILD_NUMBER defaults to "local" when TAG/BUILD_NUMBER would otherwise be empty outside Jenkins. (213d503)
There was a problem hiding this comment.
I think I missed that BUILD_NUMBER might be ${TAG}, sorry!
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
amwolff
left a comment
There was a problem hiding this comment.
please make sure that you test everything end-to-end (like you did at the beginning) before submitting next patches!
testsuite/ceph-s3-tests/run.sh
Outdated
| apt-get update && apt-get -yqq install --no-install-recommends git tox \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
you might be able to simplify this to apt update -y && apt install -y tox
There was a problem hiding this comment.
Updated the ceph s3 test setup to apt update -y && apt install -y git tox as suggested. (7a44ff1)
Jenkinsfile
Outdated
| } | ||
| steps { | ||
| sh 'make lint' | ||
| sh 'make verify-lint' |
There was a problem hiding this comment.
at this point, we can leave this as make lint (we don't need yet another target that's essentially the same)
There was a problem hiding this comment.
Lint stage now calls make lint again to keep the original behavior. (7a44ff1)
Jenkinsfile
Outdated
| stage('Cross-Vet') { | ||
| steps { | ||
| sh 'make cross-vet' | ||
| sh 'make verify-cross-vet' |
There was a problem hiding this comment.
Cross-Vet stage restored to make cross-vet. (7a44ff1)
Jenkinsfile
Outdated
| JSON = true | ||
| SHORT = false | ||
| SKIP_TESTSUITE = true | ||
| STORJ_TEST_COCKROACH = 'cockroach://root@localhost:26257/postgres?sslmode=disable' | ||
| STORJ_TEST_POSTGRES = 'postgres://postgres@localhost/postgres?sslmode=disable' | ||
| STORJ_TEST_LOG_LEVEL = 'info' | ||
| } | ||
| steps { | ||
| sh 'make test 2>&1 | grep "^{.*" | tee .build/tests.json | xunit -out .build/tests.xml' | ||
| sh 'make verify-test' |
There was a problem hiding this comment.
I think we might actually want to leave these as they were since it's unlikely that the equivalent of make test 2>&1 | grep "^{.*" | tee .build/tests.json | xunit -out .build/tests.xml will be run locally.
There was a problem hiding this comment.
Test stage restored to the original make test | tee .build/tests.json | xunit pipeline. (7a44ff1)
Jenkinsfile
Outdated
| JSON = true | ||
| SHORT = false | ||
| STORJ_TEST_COCKROACH = 'cockroach://root@localhost:26257/postgres?sslmode=disable' | ||
| STORJ_TEST_POSTGRES = 'postgres://postgres@localhost/postgres?sslmode=disable' | ||
| STORJ_TEST_LOG_LEVEL = 'info' | ||
| } | ||
| steps { | ||
| // exhaust ports from 1024 to 10000 to ensure we don't | ||
| // use hardcoded ports | ||
| sh 'use-ports -from 1024 -to 10000 &' | ||
| sh 'make --no-print-directory test-testsuite 2>&1 | tee .build/testsuite.json | xunit -out .build/testsuite.xml' | ||
| sh 'make verify-testsuite' |
There was a problem hiding this comment.
Testsuite stage restored to use-ports + make test-testsuite | xunit as before. (7a44ff1)
Makefile
Outdated
|
|
||
| ##@ Local development/Public Jenkins/Helpers | ||
|
|
||
| STORJ_UP_VERSION ?= v1.2.11 |
There was a problem hiding this comment.
any chance we can not hardcode the storj-up version?
There was a problem hiding this comment.
Switched STORJ_UP_VERSION back to main so it’s no longer pinned. (7a44ff1)
| ifneq ($(wildcard $(GOLANGCI_LINT_CONFIG)),) | ||
| GOLANGCI_LINT_CONFIG_ARG := --config ${GOLANGCI_LINT_CONFIG} | ||
| endif | ||
| ifneq ($(wildcard $(GOLANGCI_LINT_CONFIG_TESTSUITE)),) | ||
| GOLANGCI_LINT_CONFIG_TESTSUITE_ARG := --config ${GOLANGCI_LINT_CONFIG_TESTSUITE} | ||
| endif |
There was a problem hiding this comment.
ah, right—good catch. BTW let's make sure that you clone the storj/ci repo for your next run to ensure that the version upgrade works with the config file it's supposed to be used with.
Makefile
Outdated
| ##@ Local development/Public Jenkins/Integration Test | ||
|
|
||
| BUILD_NUMBER ?= ${TAG} | ||
| INTEGRATION_COMPOSE_PROJECT ?= integration-${BUILD_NUMBER} |
There was a problem hiding this comment.
I think I missed that BUILD_NUMBER might be ${TAG}, sorry!
|
|
||
| BUILD_NUMBER ?= ${TAG} | ||
| INTEGRATION_COMPOSE_PROJECT ?= integration-${BUILD_NUMBER} | ||
| INTEGRATION_BUILD_DIR ?= .build |
There was a problem hiding this comment.
should we make other scripts clean up after themselves as well?
Makefile
Outdated
|
|
||
| .PHONY: integration-env-deps | ||
| integration-env-deps: | ||
| @if ! command -v storj-up >/dev/null 2>&1; then go install storj.io/storj-up@${STORJ_UP_VERSION}; fi |
There was a problem hiding this comment.
we can likely just always go install since that's essentially going to be an idempotent op.
There was a problem hiding this comment.
integration-env-deps now always runs go install since it’s idempotent. (7a44ff1)
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
Ran end-to-end: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
amwolff
left a comment
There was a problem hiding this comment.
you should be able to access img.dev.storj.io now
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
Retried
Same update: integration-run was reattempted with registry access; the latest Dockerfile patching fix is in c408310 and needs a fresh run to confirm. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: amwolff <30234756+amwolff@users.noreply.github.com>
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.