NotificationsYou must be signed in to change notification settings
Fork1k
Star11.2k

refactor: Add install script for coder CLI#243

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bryphe-coder merged 5 commits intomainfrombryphe/refactor/add-install-script

Feb 11, 2022

Merged

refactor: Add install script for coder CLI#243

bryphe-coder merged 5 commits intomainfrombryphe/refactor/add-install-script

Feb 11, 2022

Conversation

Copy link

Contributor

bryphe-coder commentedFeb 10, 2022

This adds aninstall.sh script at the root which runsgo install cmd/coder/main.go - makingcoder available by default in our workspaces (where thego bin folder is already in thePATH).

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

bryphe-coder added2 commits

February 10, 2022 19:01

Add an ./install.sh as a helper for working with the local coder exec…

7f917af

…utable

Fix semantic commit

00ca4ce

bryphe-coder self-assigned this

Feb 10, 2022

Copy link

codecovbot commentedFeb 10, 2022•
edited
Loading

Codecov Report

Merging#243 (1b55786) intomain (a82fb8f) willdecrease coverage by0.29%.
The diff coverage isn/a.

@@            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 byCodecov. Last updatea82fb8f...1b55786. Read thecomment docs.

bryphe-coder requested a review fromkylecarbs

February 10, 2022 19:15

kylecarbs approved these changes

Feb 10, 2022

View reviewed changes

Copy link

Member

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 ourmake build, then even copy all binaries in./bin to$(go env GOPATH)/bin.

I'm concerned about users thinkinginstall.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

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

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

ContributorAuthor

Choose a reason for hiding this comment

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

I switched to usemake install here:0b16731 - and made the extension matching in4415638

Our current strategy (having./develop.sh and aMakefile) 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.

Copy link

ContributorAuthor

bryphe-coder commentedFeb 10, 2022•
edited
Loading

Should we make this a Makefile target instead?

Sure! I think the canonical way formake is to have amake install target - this could do the copy operation you mentioned (depend on thebuild target, and then copy everything toGOPATH 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.