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

build: auto-run generation and submit prs #1905

Merged
merged 6 commits into from
Feb 28, 2020
Merged

build: auto-run generation and submit prs #1905

merged 6 commits into from
Feb 28, 2020

Conversation

JustinBeckwith
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 9, 2019
synth.py Outdated Show resolved Hide resolved
@JustinBeckwith JustinBeckwith requested a review from bcoe December 9, 2019 00:14
@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #1905 into master will decrease coverage by 2.59%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1905     +/-   ##
=========================================
- Coverage    40.5%   37.91%   -2.6%     
=========================================
  Files           5        6      +1     
  Lines         995     1063     +68     
  Branches        7        7             
=========================================
  Hits          403      403             
- Misses        592      660     +68
Impacted Files Coverage Δ
src/generator/synth.ts 0% <0%> (ø)

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 197b019...78a17bb. Read the comment docs.

Copy link
Contributor

@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.

I like the idea of running this as part of sythtool, but my gut is that it might just be better to let synthtool create the PR, if we want to run this through synthtool; we have projects in the works to track PRs through the whole stack, and it feels a bit weird that this would have its own approach.

I'm pretty sure we'll know that the PRs that change the whole apis directory came from API generation, is my thinking.

@JustinBeckwith
Copy link
Contributor Author

Aye, thought had occurred to me as well. A few thoughts:

  • The work to track PRs through the whole stack will largely only apply to GAPICs :/
  • To use synthtool as of today, it would still be generating monolithic giant PRs
  • To make this work with synthtool with individual PRs, I think we'd need to start (comically) generating synth files in each individual directory. Then, we'd need a top level script (very much like this one) responsible for adding new APIs, and deleting ones that went away. It would be a pretty big rewrite of the generator code.
  • That sounded like a lot of work, and this got me 90% of where I wanted to go for much less code

The reason I suddenly started to care - I want to start releasing scoped mini-packages (finally). I started trying to help people solve the angular and react bugs, and at the end of the day we're generating 3.4MB of JavaScript that unsuspecting souls are serving the from the client. Until we have mini-packages ... it's just not going to be reasonable.

All that having been said - if you're interested - may be worth chatting with @chingor13 to see how he solved synth+apiary+mini-packages, since I'm pretty sure they have this rolling well in Java land.

@bcoe bcoe added status: blocked Resolving the issue is dependent on other work. and removed status: blocked Resolving the issue is dependent on other work. labels Dec 12, 2019
@bcoe
Copy link
Contributor

bcoe commented Dec 12, 2019

If we're going to make publishing individual clients a priority, I think it would be good for us to pull together a design document, so that I can better understand the goals (I want to be careful about adding such a big change organically; questions come up for me like, do we intend to create dozens of individual npm modules, if so, in what namespace, and will they create confusion with our GAPIC libraries).

Also, I'd love to solve the problem of generating PRs in a more holistic way; Could we have a GitHub App that runs both this generation, and our template generation, perhaps updates to the micro-generator as well? (decoupling syntactic changes from upstream API changes).

@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@7ca3b7b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1905   +/-   ##
=========================================
  Coverage          ?   37.91%           
=========================================
  Files             ?        6           
  Lines             ?     1063           
  Branches          ?        7           
=========================================
  Hits              ?      403           
  Misses            ?      660           
  Partials          ?        0

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 7ca3b7b...f121b0e. Read the comment docs.

@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2020

If you're feeling good about this approach, I'm comfortable with giving it a shot; we probably shouldn't land until we have some cycles to debug any issues though?

@JustinBeckwith
Copy link
Contributor Author

I am comfortable with at least trying it - we can always turn it off :) Honestly the thing I'm most scared about is breaking all of synth in nodejs (again) due to timeouts on the synth job.

Worst case scenario - it's busted and we just close all the PRs and turn it off 🤷‍♂ Happy to work out the kinks though.

One last thing - I'd like take your inch and turn it into a mile. How do you feel about the github action I just sprinkled in?

@bcoe
Copy link
Contributor

bcoe commented Jan 15, 2020

@JustinBeckwith if we have concerns about timeouts in synthtool, I think using a GitHub action might be a good experiment; I believe its timeout is 360 minutes?

@bcoe bcoe mentioned this pull request Jan 15, 2020
@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020
@JustinBeckwith JustinBeckwith merged commit 94410ba into master Feb 28, 2020
@JustinBeckwith JustinBeckwith deleted the submitprs branch December 13, 2020 18:45
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.

5 participants