Skip to content

Conversation

@jgehrcke
Copy link
Collaborator

This change removes a (by now) superfluous arg. The removal simplifies dependabot-automated Golang bumps.

Note that GOLANG_VERSION is injected (based on the current source of truth) when invoking for example make -f deployments/container/Makefile build.

Connecting dots: #324 (comment)

@copy-pr-bot
Copy link

copy-pr-bot bot commented May 13, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ArangoGutierrez ArangoGutierrez requested a review from Copilot May 13, 2025 13:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes a superfluous default ARG from the Dockerfile to simplify automated Golang version bumps by depending on an externally provided value. The changes include removing the hardcoded GOLANG_VERSION during the build stage, explicitly requiring the build argument, and standardizing ENV variable syntax.

Comments suppressed due to low confidence (1)

deployments/container/Dockerfile:24

  • The placeholder value 'x.x.x' for GOLANG_VERSION may lead to unintended behavior if not overridden. Consider updating the placeholder or adding a clarifying comment to ensure it's always injected via the build system.
ARG GOLANG_VERSION=x.x.x

ENV GOPATH /go
ENV PATH $GOPATH/bin:/usr/local/go/bin:$PATH
ENV GOPATH=/go
ENV PATH=$GOPATH/bin:/usr/local/go/bin:$PATH
Copy link
Collaborator Author

@jgehrcke jgehrcke May 13, 2025

Choose a reason for hiding this comment

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

(noop change, that was just my local Dockerfile linter complaining)

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Does this also require changes to the README or do we not document building using the Dockerfile directly?

@guptaNswati
Copy link
Contributor

Does this also require changes to the README or do we not document building using the Dockerfile directly?

We don't. We only document using the scripts.

@jgehrcke jgehrcke merged commit 0f1166a into NVIDIA:main May 13, 2025
7 checks passed
@klueska klueska added this to the v25.3.0 milestone Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

5 participants