Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

When fetching packages with go get, remove redundant go install #588

Merged
merged 1 commit into from
Aug 15, 2017

Conversation

bradleyfalzon
Copy link
Contributor

@bradleyfalzon bradleyfalzon commented Aug 15, 2017

"go get" fetches and installs the specified package according to the
documentation:

Get downloads the packages named by the import paths, along with their
dependencies. It then installs the named packages, like 'go install'.

Testing this myself on go1.8.3 and go1.9rc2 having an older version of
buffalo binary install, then running:

go get -u github.com/gobuffalo/buffalo/...

Shows the binary is also updated successfully when checking the version.

Relates to documentation update: gobuffalo/docs#57

Further, this appears to have first started with 1bcb981#diff-00e5b9d3695f2772e4d4422b4fee4787R92 where there's a glide get of github.com/markbates/refresh followed by go install of the same package. Could this have initially been a glide vs go behaviour difference that continued through the various refactors? But it seems safe to remove the additional go install now.

I have tested this on a basic buffalo new <name>, but I haven't extensively tested this change in regards to the soda related changes or Dockerfile.

Copy link
Member

@markbates markbates left a comment

Choose a reason for hiding this comment

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

Can you please change the base for this PR to the development branch? Thank you.

Dockerfile Outdated
@@ -17,8 +17,6 @@ ADD . .

RUN go get -v -t $(go list ./... | grep -v /vendor/)

RUN go install -v ./buffalo

Copy link
Member

Choose a reason for hiding this comment

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

This is needed. We run our tests using this dockerfile so we want to make sure we are running the tests using the code that was just committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but are you certain it's needed? The go get should be handling this. But I can add it back, no issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely needed. It's compiling the local source.

OK, but I've tested by adding a panic to new generator and the buffalo new commands started to panic. The go get should still install the local version.

But I'll add it back now.

@markbates
Copy link
Member

Definitely needed. It's compiling the local source.

@bradleyfalzon bradleyfalzon changed the base branch from master to development August 15, 2017 01:45
"go get" fetches and installs the specified package according to the
documentation:

    Get downloads the packages named by the import paths, along with their
    dependencies. It then installs the named packages, like 'go install'.

Testing this myself on go1.8.3 and go1.9rc2 having an older version of
buffalo binary install, then running:

    go get -u github.com/gobuffalo/buffalo/...

Shows the binary is also updated successfully when checking the version.
@bradleyfalzon
Copy link
Contributor Author

I've changed the base branch and removed the Dockerfile modifications, ready for a second review.

@markbates markbates merged commit 8180994 into gobuffalo:development Aug 15, 2017
@markbates
Copy link
Member

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants