-
Notifications
You must be signed in to change notification settings - Fork 244
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
Change version checker to use rawURL instead of calling API #593
Change version checker to use rawURL instead of calling API #593
Conversation
can we remove version from |
I don't know about this. The API implementation seems a lot cleaner than using raw URL's. APIs would (IMO) be more stable than relying on the raw URL. How big is this library? I think we should just keep it in. |
The problem is not about the library. |
It might be better to have two separate values. Kepp version.go as it is and create new file called Otherwise, there might be a problem when doing a release:
if we use the value from |
add file to track latest release version as mentioned in redhat-developer#593 (comment)
add a file to track latest release version as mentioned in redhat-developer#593 (comment)
@surajnarwade yes that sounds good, am adding a file for tracking the latest version in PR #613 |
@syamgk I can't build binary from source:
|
pkg/notify/notify.go
Outdated
@@ -11,7 +11,7 @@ import ( | |||
|
|||
const ( | |||
// URL to fetch latest version number | |||
VersionFetchURL = "https://raw.githubusercontent.com/redhat-developer/odo/master/cmd/version.go" | |||
VersionFetchURL = "https://raw.githubusercontent.com/redhat-developer/odo/master/build/VERSION" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@surajnarwade can you try once now? |
can you please look into this line:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
1545d3d
to
cbdc724
Compare
squashed and ready to merge now! |
Modify getLatestReleaseTag function to fetch version using raw.github* URL
cbdc724
to
5fe73ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can add a small test to getLatestReleaseTag
? I think that function is pretty important since if it fails for any reason in the future, I'd error out Odo each time a user would fetch the newest release.
pkg/notify/notify.go
Outdated
if err != nil { | ||
return "", errors.Wrap(err, "error getting latest release") | ||
} | ||
return *release.TagName, nil | ||
version_string := string(body) | ||
version_string = strings.TrimSuffix(version_string, "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be refactored to:
return strings.TrimSuffix(string(body), "\n"), nil
+ rephrase version tag stripping
b7fbf28
to
35b3185
Compare
Thanks for writing the test. If you can please re-base to latest master, that'd be great! After all tests have passed, this LGTM / can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM && Merging 😃
Modify getLatestReleaseTag function to
fetch version using raw.github* URL