Skip to content

build(node_library): migrate to Trampoline V2#762

Merged
bcoe merged 4 commits intogoogleapis:masterfrom
tmatsuo:nodejs-trampoline-v2
Oct 2, 2020
Merged

build(node_library): migrate to Trampoline V2#762
bcoe merged 4 commits intogoogleapis:masterfrom
tmatsuo:nodejs-trampoline-v2

Conversation

@tmatsuo
Copy link
Copy Markdown
Contributor

@tmatsuo tmatsuo commented Sep 18, 2020

@tmatsuo tmatsuo requested a review from bcoe September 18, 2020 17:38
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 18, 2020
SurferJeffAtGoogle
SurferJeffAtGoogle suggested changes Sep 22, 2020
View reviewed changes
Comment thread synthtool/gcp/templates/node_library/.kokoro/trampoline_v2.sh Outdated
# Defaults to /workspace.
# Potentially there are some repo specific envvars in .trampolinerc in
# the project root.

Copy link
Copy Markdown
Contributor

@SurferJeffAtGoogle SurferJeffAtGoogle Sep 22, 2020

Choose a reason for hiding this comment

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

Please add:

  1. A example of a call the this script.
  2. A description of how it fits into the larger pipeline. Does an automated tool call this script? Or a human? When? Who consumers the output docker file?

Copy link
Copy Markdown
Contributor Author

@tmatsuo tmatsuo Sep 23, 2020

Choose a reason for hiding this comment

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

Done

Comment thread synthtool/gcp/templates/node_library/.kokoro/trampoline.sh
@@ -1,28 +0,0 @@
#!/bin/bash
Copy link
Copy Markdown
Contributor

@SurferJeffAtGoogle SurferJeffAtGoogle Sep 22, 2020

Choose a reason for hiding this comment

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

To make this PR easier to rollback, please don't delete this file yet. Please add a comment describing that it's obsolete and add a TODO() to delete it.

Copy link
Copy Markdown
Contributor Author

@tmatsuo tmatsuo Sep 23, 2020

Choose a reason for hiding this comment

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

Done

Comment thread synthtool/gcp/templates/node_library/.kokoro/populate-secrets.sh
function msg { println "$*" >&2 ;}
function println { printf '%s\n' "$(now) $*" ;}

# Populates requested secrets set in SECRET_MANAGER_KEYS
Copy link
Copy Markdown
Contributor

@SurferJeffAtGoogle SurferJeffAtGoogle Sep 22, 2020

Choose a reason for hiding this comment

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

Who do we expect to call this script? When? Why? Please add comments around line 15.

Copy link
Copy Markdown
Contributor Author

@tmatsuo tmatsuo Sep 23, 2020

Choose a reason for hiding this comment

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

Comment added

Copy link
Copy Markdown
Contributor

@SurferJeffAtGoogle SurferJeffAtGoogle Sep 23, 2020

Choose a reason for hiding this comment

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

Looks good.

bcoe
bcoe approved these changes Sep 22, 2020
View reviewed changes
Copy link
Copy Markdown

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

This looks good to me, and has been tested here:

googleapis/nodejs-phishing-protection#202

I would like to do an end to end test which actually exercises the secrets, once @SurferJeffAtGoogle's comments are addressed.

@tmatsuo tmatsuo force-pushed the nodejs-trampoline-v2 branch from 9a96032 to d896213 Compare September 23, 2020 16:22
@tmatsuo
Copy link
Copy Markdown
Contributor Author

tmatsuo commented Sep 23, 2020

@SurferJeffAtGoogle Thanks for your review. PTAL

SurferJeffAtGoogle
SurferJeffAtGoogle approved these changes Sep 23, 2020
View reviewed changes
Comment thread synthtool/gcp/templates/node_library/.kokoro/populate-secrets.sh
function msg { println "$*" >&2 ;}
function println { printf '%s\n' "$(now) $*" ;}

# Populates requested secrets set in SECRET_MANAGER_KEYS
Copy link
Copy Markdown
Contributor

@SurferJeffAtGoogle SurferJeffAtGoogle Sep 23, 2020

Choose a reason for hiding this comment

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

Looks good.

@tmatsuo
Copy link
Copy Markdown
Contributor Author

tmatsuo commented Sep 23, 2020

@bcoe Now it's approved. Let me know how the e2e test goes.

@bcoe bcoe added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 25, 2020
@bcoe
Copy link
Copy Markdown

bcoe commented Sep 25, 2020

@tmatsuo I will do an end to end test Monday 👌 (_as I don't want to release a library on Friday).

@bcoe
Copy link
Copy Markdown

bcoe commented Oct 1, 2020

@tmatsuo sorry for the delay, my PRs got a bit away from me this week. I have tested this branch, and have good news and bad news.

trampoline 2.0 is mostly working, in that s publish happened:

npm:

https://www.npmjs.com/package/@google-cloud/phishing-protection

kokoro:

https://fusion.corp.google.com/projectanalysis/summary/KOKORO/prod:cloud-devrel%2Fclient-libraries%2Fnodejs%2Frelease%2Fgoogleapis%2Fnodejs-phishing-protection%2Fpublish

However, the final step never happened which updates tags, the command can be found here:

https://github.com/googleapis/nodejs-phishing-protection/blob/master/.kokoro/publish.sh#L23

It received the error:

"No github token or PR specified to report status to, returning."

@bcoe
Copy link
Copy Markdown

bcoe commented Oct 1, 2020

Is there a chance that AUTORELEASE_PR would not be set with trampoline 2.0?

https://github.com/googleapis/releasetool/blob/ff891a41001f95ed10046adf252bad3bb1b289a8/releasetool/__main__.py#L158

@tmatsuo
Copy link
Copy Markdown
Contributor Author

tmatsuo commented Oct 1, 2020

@bcoe Maybe. Who set the AUTORELEASE_PR in the first place?

@tmatsuo
Copy link
Copy Markdown
Contributor Author

tmatsuo commented Oct 1, 2020

@bcoe I see AUTORELEASE_PR is extracted from Kokoro request. I think we need to pass down this envvar into the container.

@tmatsuo tmatsuo force-pushed the nodejs-trampoline-v2 branch from 2289704 to ff83218 Compare October 1, 2020 16:46
@tmatsuo
Copy link
Copy Markdown
Contributor Author

tmatsuo commented Oct 1, 2020

@bcoe I think the last commit should solve the issue. I'll appreciate it if you can try the release again.

@bcoe bcoe removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 2, 2020
@bcoe bcoe changed the title chore(node_library): migrate to Trampoline V2 build(node_library): migrate to Trampoline V2 Oct 2, 2020
@bcoe bcoe merged commit 0c868d4 into googleapis:master Oct 2, 2020
@tmatsuo tmatsuo deleted the nodejs-trampoline-v2 branch October 2, 2020 19:13
@tmatsuo tmatsuo mentioned this pull request Oct 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chingor13 chingor13 Awaiting requested review from chingor13

2 more reviewers

@bcoe bcoe bcoe approved these changes

@SurferJeffAtGoogle SurferJeffAtGoogle SurferJeffAtGoogle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tmatsuo @bcoe @SurferJeffAtGoogle