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

Moving version from variable to text file #714

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

surajnarwade
Copy link
Contributor

Resolves #712 , As it is needed for fabric8-maven-plugin

@kompose-bot
Copy link
Collaborator

@surajnarwade, thank you for the pull request! We'll ping some people to review your PR. @cdrage and @kadel, please review this.

@kompose-bot kompose-bot requested review from cdrage and kadel July 17, 2017 11:49
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2017
@surajnarwade
Copy link
Contributor Author

cc: @surajssd for review

@cdrage
Copy link
Member

cdrage commented Jul 17, 2017

@surajnarwade

I really don't like this and this over-complicates things.

What happens if someone's developing in a different path?

What happens if someone doesn't have the source code?

What if GOPATH isn't set (they're just using Kompose on a production machine?)

You're also not checking to see if this errors out:

version, _ := ioutil.ReadFile(path)

Let's not modify version.go and only add VERSION to /build, we can modify the release.sh script to make the variable substitution of build/VERSION. But let's keep version.go as it is since this PR over-complicates gathering the version information when it could simply be a const variable.

@surajnarwade
Copy link
Contributor Author

@cdrage got it, made changes accordingly

@cdrage
Copy link
Member

cdrage commented Jul 17, 2017

LGTM

@surajssd surajssd merged commit 9a61872 into kubernetes:master Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants