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(go): add support for releasing yoshi gapic go libraries #514

Merged
merged 8 commits into from
Aug 12, 2020
Merged

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 5, 2020

This adds support for releasing the Yoshi gapic go libraries.

TODO:

  • get the CHANGES.md closer to the current style.
  • properly build up a combined gapic PR, as we dedupe it.
  • setup the configuration in the go repository, so this actually works.

Future Work:

  • figure out the manual yoshi libraries.

CC: @codyoss

@bcoe bcoe requested a review from a team as a code owner August 5, 2020 23:35
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 5, 2020
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #514 into master will increase coverage by 0.41%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
+ Coverage   81.24%   81.66%   +0.41%     
==========================================
  Files          41       42       +1     
  Lines        4815     4973     +158     
  Branches      407      400       -7     
==========================================
+ Hits         3912     4061     +149     
- Misses        902      911       +9     
  Partials        1        1              
Impacted Files Coverage Δ
src/releasers/go-yoshi.ts 94.23% <94.23%> (ø)
src/conventional-commits.ts 98.37% <100.00%> (+0.01%) ⬆️

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 a6c9e0b...daff3b6. Read the comment docs.

src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved

// These repos should be ignored when generating a release for the
// top level mono-repo:
const IGNORED_PATHS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this for now, but ... I do wonder out loud what a more sustainable model for release-please looks like. I like the idea of built-in support for monorepo setups, and for split repo setups across languages. For cases where there is very customer configuration like this, I'd very much like to start thinking about a config format you could use so this code can remain more generalized. This isn't blocking feedback for this PR, just a seed to plant.

Copy link
Member

Choose a reason for hiding this comment

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

I think we will eventually want this to be programatic, especially if we ever start split out into more and more sub-modules. But for now, this "works" for the top level module at least. I am still grokking what this code has context to. I have some ideas in the back of my head how we could make this better in the future. For 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.

so, we do have some support for mono-repos now:

#501

But I was thinking this logic is a little yoshi specific...

What I've been thinking would be nice, would be to keep these yoshi specific strategies somewhat hidden, i.e., not calling them out in documentation. And, when we add a strategy like yoshi-java, or yoshi-go, it would be nice to also add corresponding strategies that are closer to something the community could use.

src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved
src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved
src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved
src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved
src/releasers/go-yoshi.ts Outdated Show resolved Hide resolved
test/releasers/yoshi-go.ts Outdated Show resolved Hide resolved
test/releasers/yoshi-go.ts Show resolved Hide resolved
@codyoss
Copy link
Member

codyoss commented Aug 9, 2020

I think this is good enough for the core parent module imo. I am not sure what is up with the failing header check, snapshots should be exempts according to the config...

@bcoe if you want to take a look again too, can't add you as a review since you opened the PR.

@codyoss codyoss requested a review from JustinBeckwith August 9, 2020 17:33
@bcoe
Copy link
Contributor Author

bcoe commented Aug 11, 2020

@codyoss we should add an exception for the __snapshot__ folder to the header checker, it wants there to be copyrights on these auto generated fixtures (it can be ignored in this case).

Copy link
Contributor Author

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

@codyoss I'll work on seeing this over the finish line sometime tomorrow, if you're happy.

__snapshots__/yoshi-go.js Show resolved Hide resolved
});

// remove commit reference from auto-generate gapics
changelogEntry = changelogEntry.replace(/\s\(\[null\].*\n/, '\n');
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 find this logic a little bit weird, I'll take a look before I land and see if we can simplify.

@codyoss
Copy link
Member

codyoss commented Aug 11, 2020

@bcoe It looks like snapshots should already be ignored if I am understanding this right.

"ignoreFiles": [
"**/__snapshots__/*"
]

@bcoe bcoe merged commit 25a234a into master Aug 12, 2020
@bcoe bcoe deleted the go-all branch August 12, 2020 06:00
@bcoe
Copy link
Contributor Author

bcoe commented Aug 12, 2020

@codyoss weird, it's __snapshots__/yoshi-go.js is missing a valid license header that's being flagged.

Should that ignore rule be **/__snapshots__/**, I'm terrible at globs.

@codyoss
Copy link
Member

codyoss commented Aug 12, 2020

@bcoe I rule looked right to me, and it works for all the other files in that directory. I think ** is only need for recursive dirs.

codyoss added a commit to googleapis/google-cloud-go that referenced this pull request Aug 13, 2020
Once merged, we can merge the connected PR and then we should
start to see proposed releases for the root module. More work will
need to be done to figure out supporting the sub-modules.

Related: googleapis/release-please#514
tritone pushed a commit to tritone/google-cloud-go that referenced this pull request Aug 25, 2020
…#2716)

Once merged, we can merge the connected PR and then we should
start to see proposed releases for the root module. More work will
need to be done to figure out supporting the sub-modules.

Related: googleapis/release-please#514
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants