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

Fix docker install instructions #28

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Fix docker install instructions #28

merged 1 commit into from
Nov 1, 2023

Conversation

cfhammill
Copy link
Contributor

The docker instructions as written do not work, but using the pure go example below I noticed it was just missing a path element. This seems to fix it for me.

README.md Outdated
@@ -155,7 +155,7 @@ ${DOCKER} run \
-v $(pwd):/dest \
golang:1.16.4 \
sh -c "
go install github.com/NVIDIA/mig-parted/cmd@latest
go install github.com/NVIDIA/mig-parted/cmd/nvidia-mig-parted@latest
mv /go/bin/cmd /dest/nvidia-mig-parted
Copy link
Member

Choose a reason for hiding this comment

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

Could it be that the cmd here is not correct? Running go install will mostlikely create a nvidia-mig-parted exectuable in the go path, so the following should be mv /go/bin/nvidia-mig-parted /dest/nvidia-mig-parted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, I've made that change and upgrade the go version in additional commits to this branch

@elezar
Copy link
Member

elezar commented Oct 31, 2023

Note that this is a read-only mirror and I have pushed this branch to our GitLab repo and created a Merge Request:

https://gitlab.com/nvidia/cloud-native/mig-parted/-/merge_requests/143

I do think this needs some changes and it would also be good if you sign-off your commit (with the -s flag).

@cfhammill
Copy link
Contributor Author

thanks @elezar, I pushed a few more changes to the branch before I saw your response, there is another pull request (#25) that fixes essentially the same code in the install script. How should I go about signing the commits, I could do a squash and sign that?

@elezar
Copy link
Member

elezar commented Oct 31, 2023

thanks @elezar, I pushed a few more changes to the branch before I saw your response, there is another pull request (#25) that fixes essentially the same code in the install script. How should I go about signing the commits, I could do a squash and sign that?

It's not required to sign the commit, just sign-off as required by the DCO. You should be able to just add something like:

Signed-off-by: Evan Lezar <[email protected]>

(with your name and email address) as the last line of the commit message. Note that each commit needs to be signed-off.

Since the commit in #25 isn't signed-off either, I would be ok with you making the changes. Alternatively we can reach out to the author to get them to do the same.

The docker instructions as written do not work, but using the pure go example below I noticed it was just missing a path element. This seems to fix it for me.

fix golang image version and bin mv

bump go version as in NVIDIA#25

use docker bin instead of likely undefined env var

remove the container after completion (--rm)

Signed-off-by: Chris Hammill <[email protected]>
@cfhammill
Copy link
Contributor Author

Thanks @elezar, probably best to reach out to that author of #25 on the PR thread.

I've force pushed a signed-off squash commit for this PR, should be ready to go. Let me know if you'd like additional changes and if you'd prefer those squashed onto this commit.

@elezar elezar merged commit 788830b into NVIDIA:main Nov 1, 2023
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.

2 participants