Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update doc and base image for Go 1.19 #1330

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

tedhtchang
Copy link
Contributor

Why are these changes needed?

Please see issue#1329

Update Development.md and Dockerfile for Go 1.19

Related issue number

Closes #1329

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

How to verify

Run the following and make sure no errors from the make commands.

git clone --single-branch --branch issue-1329 https://github.com/tedhtchang/kuberay.git
cd kuberay/proto
export IMAGE_NAME=<registry>/proto-generator # For example: docker.io/tedhtchang/proto-generator
export IMAGE_TAG=test
export PREBUILT_REMOTE_IMAGE=${IMAGE_NAME}:${IMAGE_TAG}
make make build-image
make generate

/cc @kevin85421 @anishasthana

@@ -33,3 +36,4 @@ ENV XDG_CACHE_HOME /tmp/.cache
RUN chmod -R 775 /usr/bin/
RUN chmod -R 775 /usr/include/google
RUN chmod -R 775 /go
USER 65532:65532
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining the reason behind this change? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed temporary root privilege to install packages. I saw this usage pattern here. @anishasthana Could you take a look if USER 65532:65532 is necessary ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it is. The alternative is that we will run the pod as root, which is pretty not-good

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@kevin85421
Copy link
Member

cc @anishasthana would you mind taking a look? Thx!

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge this PR after @anishasthana approves this PR.

Copy link
Contributor

@anishasthana anishasthana left a comment

Choose a reason for hiding this comment

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

Changes look good. Thanks @tedhtchang!

@kevin85421 kevin85421 merged commit aae9fac into ray-project:master Aug 15, 2023
20 checks passed
blublinsky pushed a commit to blublinsky/kuberay that referenced this pull request Aug 22, 2023
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
Update doc and base image for Go 1.19
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.

Update doc / base image for Go 1.19
3 participants