-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add GH release
workflow and bump-version
action, fix hard-coded client version
#66
Conversation
…icit file, add new github action for bump-version allowing us to easily bump a semver version with a prereleaseSuffix if desired, add new release github workflow to facillitate triggering releases for the Go sdk using git tags and updating the version.go file
@@ -0,0 +1 @@ | |||
node_modules/ |
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 entire action was lifted from work @jhamon did previously: pinecone-io/pinecone-python-client#178
I'll call out lines I modified, which should be within action.js
and index.js
where I got rid of modifying local files via fs.readFileSync
and fs.writeFileSync
. I'm doing the file manipulating using sed
in the release.yaml
file.
core.setOutput("version_tag", `v${newVersion}`); | ||
} |
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.
I got rid of the return newVersion
here since we're not using it in index.js
.
|
||
action.bumpVersion( | ||
core.getInput("currentVersion"), | ||
core.getInput("bumpType"), | ||
core.getInput("prereleaseSuffix") | ||
); |
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.
Got rid of fs.readFileSynx
, fs.writeFileSynx
, and doing anything with the newVersion
that used to get returned from the action.bumpVersion
call. bumpVersion
uses core.setOutput
and then we use the output in the release.yaml
workflow.
@@ -0,0 +1,3770 @@ | |||
{ |
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.
A bummer, but if we can move all of this junk into its own public repo and then reuse the action across our repos that would be lovely.
) | ||
|
||
func getPackageVersion() string { | ||
// update at release time | ||
return "v0.5.0" | ||
return internal.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.
This is the only fix to the code itself, and we're expecting the internal.Version
value to be somewhat managed through the CI process.
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.
nice!
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.
👍
) | ||
|
||
func getPackageVersion() string { | ||
// update at release time | ||
return "v0.5.0" | ||
return internal.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.
nice!
@@ -3,11 +3,12 @@ package useragent | |||
import ( | |||
"fmt" | |||
"strings" | |||
|
|||
"github.com/pinecone-io/go-pinecone/internal" |
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.
isn't useragent already in this package?
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.
For Go, I don't think internal
was explicitly a package until I added a file referencing package internal
. useragent
, control
, data
, and provider
are all separate packages that are defined under /internal/
in the ,
repo but there's no inheritance or anything like that in Go, so you still need to explicitly pull in the internal package. At least that's my understanding.
const delimiter = `delimiter_${Math.floor(Math.random() * 100000)}`; | ||
const convertedValue = toCommandValue(value); | ||
|
||
// These should realistically never happen, but just in case someone finds a |
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.
nice
Problem
We have a private function which returns a hard-coded version number which is used when we build the user-agent for requests originating from the client. This is difficult to maintain while releasing because Go releases are handled through GitHub and Git tags, and requires us to keep things in sync manually.
I also missed updating this for
v1.0.0
, so it needs to be bumped anyways.We're also missing any kind of "official" release process for the Go client, which would be beneficial in standardizing versioning and the process for releasing the client now that we're >v1.0.0. I'd rather we have a GitHub workflow similar to the other clients even if it primarily involves bumping a hard-coded version file, and pushing a tag.
Solution
internal/version.go
file which packages and holdsinternal.Version
to be used within the client. This is the file that CI will manage and commit updates towards as we bump versions..github/actions/bump-version
allowing us to easily bump thecurrentVersion
using a specificreleaseLevel
(major, minor, patch),isPrerelease
, andprereleaseSuffix
if desired. This action was something @jhamon originally added to the pinecone-python-client. I've lifted it here with minor alterations to support just returning a newversion_tag
rather than modifying a file usingfs
which it does in the Python repo../github/workflows/release.yaml
to facilitate releasing the Go SDK by updatinginternal/version.go
, and pushing the new tag. For prerelease, we just push the tag without committing.Type of Change
Test
I'll need to get the CI files into source before I can test the actual release process. My thinking here is I've updated the hard-coded value to the current
v1.0.0
, and I'd like to release av1.0.1
to fix the current problem with the user-agent not matching. I think this is reasonable, but let me know if you don't agree.The
bump-version
action itself was lifted directly from @jhamon's work including unit tests, so I think that is mostly safe. I'd spend the bulk of the review looking at my approach inrelease.yaml
.