Skip to content

test build-with-token - #4516

Open
dtrawins wants to merge 14 commits into
mainfrom
build-with-token
Open

test build-with-token#4516
dtrawins wants to merge 14 commits into
mainfrom
build-with-token

Conversation

@dtrawins

@dtrawins dtrawins commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

🛠 Summary

optional usage of github TOKEN to pull dependencies using authenticated client
json and opencv switched from git clone to downloading tar.gz with src
this PR should mitigate github instability

🧪 Checklist

  • Unit tests added.
  • The documentation updated.
  • Change follows security best practices.
    ``

Comment thread ci/build_test_OnCommit.groovy Outdated
@dtrawins
dtrawins marked this pull request as ready for review September 8, 2026 06:36
@dtrawins
dtrawins requested review from atobiszei and a lite review from Copilot September 8, 2026 06:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces avoidable secret-exposure risks (logging the environment inside withCredentials, and writing a token-bearing .gitconfig into the Docker build context) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the build pipeline to support authenticated GitHub fetches during Docker builds (via a token-backed gitconfig secret), while also adjusting a few third-party dependency fetch mechanisms for more deterministic builds.

Changes:

  • Add Makefile support for passing a gitconfig as a Docker BuildKit secret when TOKEN is provided.
  • Update Dockerfiles to mount the gitconfig secret during git clone/Bazel steps to enable authenticated fetches.
  • Update dependency pins/fetch methods in WORKSPACE and switch OpenCV install script from git clone to tag tarball downloads.
File summaries
File Description
WORKSPACE Updates mediapipe pin and adjusts dependency fetching (node rules URL change; nlohmann_json to http_archive).
third_party/opencv/install_opencv.sh Switches OpenCV sources acquisition from git clone to tarball extraction.
Makefile Adds token-based gitconfig generation and passes it into Docker builds as a secret.
Dockerfile.ubuntu Uses BuildKit secret mounts for git operations and Bazel builds.
Dockerfile.redhat Uses BuildKit secret mounts for git operations and Bazel builds.
ci/build_test_OnCommit.groovy Wraps Linux image build steps with Jenkins credentials binding for TOKEN.
.gitignore Ignores generated .gitconfig.
Review details

Suppressed comments (1)

Makefile:223

  • .gitconfig contains a GitHub access token; it should be created with restrictive permissions to reduce the chance of accidental disclosure on shared machines/workspaces. Setting a tight umask (or chmod 600) when writing the file avoids it being created world-readable under a permissive umask.
$(GIT_CONFIG_FILE):
	@git config --file $@ url."https://x-access-token:$(TOKEN)@github.com/".insteadOf https://github.com/
  • Files reviewed: 6/7 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Makefile
Comment on lines +217 to +219
ifneq ($(TOKEN),)
GIT_CONFIG_FILE := .gitconfig
GIT_CONFIG_SECRET = --secret id=gitconfig,src=$(GIT_CONFIG_FILE)
Comment on lines +234 to +236
withCredentials([usernamePassword(credentialsId: 'workflow_lab_mediapipe', usernameVariable: 'GIT_USERNAME', passwordVariable: 'TOKEN')]) {
sh "env"
sh "make ovms_builder_image RUN_TESTS=${runTestsFlag} OPTIMIZE_BUILDING_TESTS=1 OVMS_CPP_IMAGE_TAG=${shortCommit} BUILD_IMAGE=openvino/model_server-build:${shortCommit}"
Comment thread WORKSPACE
Comment on lines 178 to 181
name = "mediapipe",
remote = "https://github.com/openvinotoolkit/mediapipe",
commit = "12e8d511cfbc5f471c498278a65a02dd250963e8", # top of mediapipe main branch as of 26.11.2025
commit = "e57eca54b6190aa482c14ddc8b53a4e7d4088ce3", # top of mediapipe main branch as of 26.11.2025
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

mediapipe change needs to be merged first?

@dtrawins
dtrawins requested review from mzegla and rasapala September 8, 2026 12:14
Comment thread WORKSPACE
)

git_repository(
http_archive(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since it's passing CI I guess its fine. If I recall there were sometimes issues with resolving dependency names when our dependency used bazel as well. Sometimes this forced us to use git_repository instead of http_archive rule.

Nothing to be done now with that. Leaving for future ourselves in case of issues,

Comment thread WORKSPACE
name = "mediapipe",
remote = "https://github.com/openvinotoolkit/mediapipe",
commit = "12e8d511cfbc5f471c498278a65a02dd250963e8", # top of mediapipe main branch as of 26.11.2025
commit = "e57eca54b6190aa482c14ddc8b53a4e7d4088ce3", # top of mediapipe main branch as of 26.11.2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

commit changed, but comment did not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants