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

refactor: Add install script for coder CLI #243

Merged
merged 5 commits into from
Feb 11, 2022

Conversation

bryphe-coder
Copy link
Contributor

This adds an install.sh script at the root which runs go install cmd/coder/main.go - making coder available by default in our workspaces (where the go bin folder is already in the PATH).

I thought this might be helpful for developer who aren't familiar with go or the directory structure, in running coder CLI locally. Open to other ideas though!

@bryphe-coder bryphe-coder self-assigned this Feb 10, 2022
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #243 (1b55786) into main (a82fb8f) will decrease coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   67.31%   67.02%   -0.30%     
==========================================
  Files         120      120              
  Lines        6484     6484              
  Branches       67       67              
==========================================
- Hits         4365     4346      -19     
- Misses       1701     1717      +16     
- Partials      418      421       +3     
Flag Coverage Δ
unittest-go-macos-latest 65.02% <ø> (-0.34%) ⬇️
unittest-go-ubuntu-latest 66.07% <ø> (-0.48%) ⬇️
unittest-go-windows-latest 65.85% <ø> (-0.30%) ⬇️
unittest-js 64.76% <ø> (ø)
Impacted Files Coverage Δ
peer/conn.go 77.69% <0.00%> (-3.85%) ⬇️
database/pubsub.go 75.00% <0.00%> (-2.09%) ⬇️
coderd/provisionerdaemons.go 52.50% <0.00%> (-0.63%) ⬇️
provisionerd/provisionerd.go 68.90% <0.00%> (-0.61%) ⬇️
peerbroker/dial.go 80.95% <0.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a82fb8f...1b55786. Read the comment docs.

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Should we make this a Makefile target instead?

It could run our make build, then even copy all binaries in ./bin to $(go env GOPATH)/bin.

I'm concerned about users thinking install.sh would install all of Coder, not just the CLI.

install.sh Outdated

go install cmd/coder/main.go
echo "Coder CLI now installed at:"
echo "$(which coder)"
Copy link
Member

Choose a reason for hiding this comment

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

We can make this more accurate with:

echo "$(go env GOPATH)/bin/coder"

Copy link
Contributor

Choose a reason for hiding this comment

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

could even do both, install to that location, check if they are different, and if so, emit a warning indicating that something else in PATH is shadowing the one we just installed

Copy link
Contributor

Choose a reason for hiding this comment

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

also, unsure whether it matters for this particular script, but if you run it on Windows, then it might not work correctly due to the .exe extension

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 switched to use make install here: 0b16731 - and made the extension matching in 4415638

Our current strategy (having ./develop.sh and a Makefile) is assuming we have some sort of POSIX toolchain in the Windows environment - like Mingw/cygwin. Might be something we have to think about fixing up later.

@bryphe-coder
Copy link
Contributor Author

bryphe-coder commented Feb 10, 2022

Should we make this a Makefile target instead?

Sure! I think the canonical way for make is to have a make install target - this could do the copy operation you mentioned (depend on the build target, and then copy everything to GOPATH bin). Does that sound good?

Still kind of sounds like it would install everything - but since we're copying all the binaries (coderd, provisionerd, coder), it's a little more accurate.

@kylecarbs
Copy link
Member

I think that'd be good! I'm just used to make install in other projects, so I have some bias here.

@bryphe-coder bryphe-coder merged commit c0d547b into main Feb 11, 2022
@bryphe-coder bryphe-coder deleted the bryphe/refactor/add-install-script branch February 11, 2022 04:32
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.

None yet

3 participants