Conversation
…t default branch during migration
google-cla
Bot
added
the
cla: yes
bcoe
left a comment
bcoe
left a comment
There was a problem hiding this comment.
This is looking pretty reasonable to me, with the caveat that I think it would be good to add a test.
I think you could use something like requests-mock so that you don't need to actually care about the traffic to GitHub.
| return json.load(f) | ||
| return {} | ||
|
|
||
| def _get_default_branch_name(package_name: str) -> str: |
|
|
||
| t = templates.TemplateGroup(self._template_root / directory, self.excludes) | ||
|
|
||
| repo = kwargs["metadata"]["repository"] |
There was a problem hiding this comment.
This looks reasonable to me, but I think it would be worth adding a test here:
| t = templates.TemplateGroup(self._template_root / directory, self.excludes) | ||
|
|
||
| repo = kwargs["metadata"]["repository"] | ||
| github_req = requests.get(f"https://api.github.com/repos/{repo}") |
There was a problem hiding this comment.
@SurferJeffAtGoogle Will this be problematic for owl-bot (as it's not hermetic)?
There was a problem hiding this comment.
I think Jeff is out til Friday? Is there anyone else we can ask about this? Or do you have thoughts on this?
There was a problem hiding this comment.
To me this seems like it's probably okay, as it will be applied in the post-processor, and will stay consistent for the repo itself?
bcoe
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
|
@billyjacobson thank you for adding this 👍 sorry it took a while to land, the beginning of the week was a bit swamped. |
|
|
||
| def test_get_default_branch(): | ||
| with requests_mock.Mocker() as m: | ||
| m.get( |
There was a problem hiding this comment.
I really appreciate that you added a test for this 🆗
|
Sorry I didn't chime in on this sooner. This is causing @sofisl and I problems because we're trying to generate templated code for a new library, that doesn't have a corresponding github repo yet. With this PR, @bcoe and @billyjacobson Can we observe or encode the default branch without making network requests? |
|
I don't know a ton about this system, but that does sound somewhat
impossible, no? To get the default branch needs a network request at some
point, so maybe there is some other way we can handle this. Perhaps Ben has
some more insights than I do.
…On Mon, May 10, 2021 at 6:24 PM Jeffrey Rennie ***@***.***> wrote:
Sorry I didn't chime in on this sooner.
This is causing @sofisl <https://github.com/sofisl> and I problems
because we're trying to generate templated code for a new library, that
doesn't have a corresponding github repo yet.
With this PR, _generic_library() fetches data from the network *during
template generation*. That's not what I imagined it would ever do. That
also means it will fail whenever the network is flaky. It's very fragile.
@bcoe <https://github.com/bcoe> and @billyjacobson
<https://github.com/billyjacobson> Can we observe or encode the default
branch without making network requests?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1031 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABL2ITYOCL6CCPBHE47UHU3TNBMLLANCNFSM42TKCYTA>
.
--
*• **Billy Jacobson*
*•* Developer Programs Engineer,
*• *Google Cloud Developer Relations
*•* 111 8th Ave, New York, NY 10011
<https://maps.google.com/?q=111+8th+Ave,+New+York,+NY+10011&entry=gmail&source=g>
|
5 participants
Add fetch for default branch so autogen files can use the correct default branch during migration