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

Move ko binary to root of project. #257

Merged
merged 7 commits into from
Dec 22, 2020
Merged

Conversation

n3wscott
Copy link
Contributor

@n3wscott n3wscott commented Dec 4, 2020

It has become clear ko and this repo is a single tool repo. Let's embrace that, and allow go get github.com/google/ko to work as 99% of folks try to do without reading the readme.

ko.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #257 (645669d) into master (5b08b05) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #257   +/-   ##
=======================================
  Coverage   37.06%   37.06%           
=======================================
  Files          33       33           
  Lines        1465     1465           
=======================================
  Hits          543      543           
  Misses        833      833           
  Partials       89       89           

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 5b08b05...645669d. Read the comment docs.


```shell
GO111MODULE=on go get github.com/google/ko/cmd/ko
go get github.com/google/ko
Copy link
Member

Choose a reason for hiding this comment

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

What should we do for users that install from cmd/ko today in scripts? This change would break them without a lot of help about what changed or how to fix it.

Can we keep cmd/ko/main.go as an alias for a release or two with a deprecation warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was always my plan to keep both. We have a lot of tooling that hardcodes installing ko from the ./cmd/ko path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change would break them without a lot of help about what changed or how to fix it.

It never broke them, I left ./cmd/ko in the PR.

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 did drop ./cmd/ko/test


# HACK HACK HACK:
# Until we can cleanly remove it, copy root main.go and move it to ./cmd/ko
cp ${PROJECT_ROOT}/main.go ${PROJECT_ROOT}/cmd/ko/
Copy link
Collaborator

Choose a reason for hiding this comment

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

@imjasonh ^

Is it possible to just commit a symlink to the root? Or would that break everything?

Copy link
Member

Choose a reason for hiding this comment

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

Missed this 😇

I'd prefer something explicit since it also gives us an opportunity to inject a deprecation warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a message, dropped the copy,

@@ -13,10 +13,10 @@

`ko` can be installed and upgraded by running:

**Note**: Golang version `1.12.0` or higher is required.
**Note**: Golang version `1.14.0` or higher is required.
Copy link
Member

Choose a reason for hiding this comment

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

Why is 1.14 required now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what the go mod file says.

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.

4 participants