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

feat: Update release-please version, add r-p config for Go, add Go publish CI #1030

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

jeremytchang
Copy link
Collaborator

@jeremytchang jeremytchang commented Mar 22, 2022

  • Updated release-please to latest v3 from v2 that contains the needed functionality for releasing Go SDK. The breaking changes should not affect us if we can trust their changelog: https://github.com/google-github-actions/release-please-action/releases/tag/v3.0.0 (I will be on call/on hand when we cut the next release to help)
  • Setup release-please config to release Go SDK at v0 only.
  • Setup release-please-manifest to point go sdk to 0.0.1. We want to keep the version below v1 or else we have to deal with major versioning naming as described above. 0.0.0 not used because release-please automatically bumps it to 1.0.0 ignoring the *-pre-major release-please configs.
  • Setup a go publish github action that publishes go module to go proxy triggered off a new go version tag. The github action is a pared down version of the python publish action.
  • Updated go.mod and go.sum with go mod tidy which cleans up dependencies

I've confirmed all my changes work locally and in my test repo at https://github.com/jeremytchang/test-release-please-go

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

nice work! I'm excited to see another language added to release-please.

couple questions

@@ -8,7 +8,7 @@ jobs:
release-please:
runs-on: ubuntu-latest
steps:
- uses: google-github-actions/release-please-action@b1f383133aa4cc90eca1d35ae7ac7d96c1e83d72
- uses: google-github-actions/release-please-action@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be awesome if this works out of the box! I wonder if there are now more configuration knobs we can tweak to support our non-standard versioning strategies and stop having to tweak release-please-config.json so much

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Afaict from reading through all the manifest and configs, our non-standard versioning where we override the conventional versioning with release-as is the accepted way of doing this. But i'll double check later too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/googleapis/release-please/blob/main/docs/manifest-releaser.md#configfile

    "path/to/myPyPkgA": {
      // when a default release-as is set, this is how you revert to using
      // conventional commits version bumping for an individual package.
      // Note: if present and set to "", the behavior will always be to use
      // conventional commits, regardless of presence or absence of default
      // release-as config.
      "release-as": "",
      "package-name": "coolio-pkg",
      // our change log is located at path/to/myPyPkgA/docs/CHANGES.rst
      "changelog-path": "docs/CHANGES.rst"
    },
    ```

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, for sure. but I remember them telling me that now that they've refactored it a bit, we can somehow plug in and setup a custom strategy like "for packageA, only ever bump the patch version by default"

@@ -15,5 +15,6 @@
"packages/sdk-node": "22.2.0",
"packages/sdk-rtl": "21.3.1",
"packages/wholly-sheet": "0.5.25",
"python": "22.2.1"
"python": "22.2.1",
"go": "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, IIRC this actually won't do anything. The way release-please works is it looks at the contents of this file at the commit that represents the last release PR merging into main, not what's actually at HEAD.

I can't remember what the changelog behavior looks like when adding a new package. The default version gets set like this BUT you'll want to over-ride it with a "release-as": "22.4.0" tag (either directly the go config in release-please-config.json or inheriting it from top level if you get into the next release)

Copy link
Collaborator Author

@jeremytchang jeremytchang Mar 22, 2022

Choose a reason for hiding this comment

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

https://github.com/googleapis/release-please/blob/main/docs/manifest-releaser.md#manifest
Once the first release PR has been merged, subsequent release-pr runs will retrieve the content of the manifest at the commit corresponding to that merged PR. It will use this to find the last-released version for each package. It will only read the manifest versions at the tip of the default/configured branch if it fails to find a package's version in the last released manifest content. It will only consider version info it is missing for a configured package (thus handling the new package bootstrap case).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we should be good. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, ok.

@@ -19,6 +19,7 @@
"packages/sdk-node": { },
"packages/sdk-rtl": { "release-as": "" },
"packages/wholly-sheet": { "release-as": "" },
"python": { "release-type": "python", "package-name": "looker_sdk" }
"python": { "release-type": "python", "package-name": "looker_sdk" },
"go": { "release-type": "go", "package-name": "go", "tag-separator": "/" }
Copy link
Contributor

Choose a reason for hiding this comment

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

woohoo!!

convert tab to space:

Suggested change
"go": { "release-type": "go", "package-name": "go", "tag-separator": "/" }
"go": { "release-type": "go", "package-name": "go", "tag-separator": "/" }

also, not sure how go package naming works but I think what you have here will literally create a tag named go/v22.4.0 and a release named go go/v22.4.0 (assuming release-please creates tags/release like it does for the other languages). Is that what you want? or maybe looker_go?

edit: now that I'm reading your publish model I see that the tag name go/v22.4.0 is what you actually want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, thanks! I love this commit suggestion UX.

And yeah, it's not ideal. I would prefer the release to be named go-sdk go/v0.0.1, but release-please is not that configurable, and I have to have the tag be in the format of go/v0.0.1 unfortunately. :(

Copy link
Collaborator Author

@jeremytchang jeremytchang Mar 22, 2022

Choose a reason for hiding this comment

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

Oh I missed adding a "release-as": "". D'oh! Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, since it's going into the next release, we'll have a top-level one anyway so no need for a specific go release-as

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad if i wasn't clear in the PR description. I'm keeping the Go SDK at v0 and making use of the -pre-major flags to keep it below v1. We're not ready for v1+ versioning for Go SDK yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, we have been calling the Go SDK GoLook if golook works as a tag

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't 😭 It has to be the subdirectory of the repo. So go and then a / and then version with a v prefixed. Argh go is super opinionated. I've done too much reading into Go versioning and their motivations. sigh

https://go.dev/ref/mod
If the module is not defined in the repository’s root directory, the module subdirectory is the part of the module path that names the directory, not including the major version suffix. This also serves as a prefix for semantic version tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's totally fine. Getting it version tagged is splendid however we have to make it work.

joeldodge79
joeldodge79 previously approved these changes Mar 22, 2022
@@ -8,7 +8,7 @@ jobs:
release-please:
runs-on: ubuntu-latest
steps:
- uses: google-github-actions/release-please-action@b1f383133aa4cc90eca1d35ae7ac7d96c1e83d72
- uses: google-github-actions/release-please-action@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, for sure. but I remember them telling me that now that they've refactored it a bit, we can somehow plug in and setup a custom strategy like "for packageA, only ever bump the patch version by default"

@github-actions

This comment has been minimized.

@jeremytchang
Copy link
Collaborator Author

@joeldodge79 You can define a custom strategy by contributing to the release-please codebase like go-yoshi which i think is a specific GCP oriented go strategy. https://github.com/googleapis/release-please/tree/main/src/strategies

@github-actions

This comment has been minimized.

jeremytchang and others added 4 commits March 23, 2022 11:00
…lish CI

- Updated release-please to latest v3 from v2 that contains the needed functionality for releasing Go SDK. The breaking changes should not affect us: https://github.com/google-github-actions/release-please-action/releases/tag/v3.0.0
- Setup release-please config to release Go SDK at v0 only.-- To release Go SDK at v1 and above requires automated editing of  the module name in go.mod during SDK generation or when release-please creates a release PR. Release-please gi
thub issue: googleapis/release-please#1344
-- The repo tag must be in the exact format of "go/vX.X.X". So package-name and tag-separator in release-please-config.json are set accordingly
- Setup release-please-manifest to point go sdk to 0.0.1. 0.0.0 is automatically bumped to 1.0.0 ignoring the "pre-major" release-please configs.
- Setup a go publish github action that publishes go module to go proxy based off the new tag
- Updated go.mod and go.sum with go mod tidy which cleans up
dependencies
Co-authored-by: Joel Dodge <joeldodge@google.com>
@jeremytchang
Copy link
Collaborator Author

@joeldodge79 I needed to rebase and it dismissed your approval. Can you approve again? Thanks for the code review and questions!

@github-actions
Copy link
Contributor

Go Tests

    7 files  ±0      7 suites  ±0   6m 1s ⏱️ -34s
  42 tests ±0    42 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
111 runs  ±0  111 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 59caa57. ± Comparison against base commit c1675ab.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM but you should wait for Joel/Mike to respond.

I know you're planning to document this package availability. One place should be in the GO SDK readme.

Copy link
Contributor

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

👍

@jeremytchang jeremytchang merged commit e597978 into main Mar 23, 2022
@jeremytchang jeremytchang deleted the jc/go_publish branch March 23, 2022 18:14
@github-actions

This comment has been minimized.

jeremytchang added a commit that referenced this pull request Mar 24, 2022
jeremytchang added a commit that referenced this pull request Mar 24, 2022
…dd Go publish CI (#1030)" (#1035)

Issue tracking the reason we can't update from v2 to v3 release-please. googleapis/release-please#1348

This reverts commit e597978.
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.

3 participants