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

Remove Examples link from release email #638

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

dougm
Copy link
Member

@dougm dougm commented Sep 21, 2018

Fixes #629

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 21, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 21, 2018
@ixdy
Copy link
Member

ixdy commented Sep 21, 2018

should we also update toolbox/relnotes/main.go? (does anything even use that program?)

@dougm
Copy link
Member Author

dougm commented Sep 21, 2018

@ixdy good question, looks like toolbox/relnotes was meant to replace the bash script, but hasn't yet been integrated? #434 (comment)

@ixdy
Copy link
Member

ixdy commented Sep 21, 2018

@dougm let's fix both then (and then tangentially figure out what to do with toolbox/relnotes).

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 22, 2018
@dougm
Copy link
Member Author

dougm commented Sep 22, 2018

@ixdy sounds good, done.

@tpepper
Copy link
Member

tpepper commented Sep 24, 2018

Change looks reasonable from a basic review perspective, but...have you tested this? Would like to know it runs ok, ahead of the final release.

@ixdy
Copy link
Member

ixdy commented Sep 24, 2018

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 24, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dougm, ixdy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2018
@@ -222,8 +222,7 @@ create_body () {
echo
echo "# $title"
echo
echo "[Documentation](https://docs.k8s.io) &" \
"[Examples](https://releases.k8s.io/$CURRENT_BRANCH/examples)"
echo "[Documentation](https://docs.k8s.io)"
Copy link
Member Author

Choose a reason for hiding this comment

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

@tpepper this hunk is the only change that applies to release-notify, run by anago here:

release/anago

Lines 445 to 449 in 920dcfb

logrun -s relnotes $RELEASE_VERSION_PRIME --release-tars=$release_tars \
--branch=${PARENT_BRANCH:-$RELEASE_BRANCH} --htmlize-md \
--markdown-file=$RELEASE_NOTES_MD \
--html-file=$RELEASE_NOTES_HTML \
--release-bucket=$RELEASE_BUCKET || return 1

The *.go changes below are not (yet) used by release-notify, though I did test them via go test.

Copy link
Member

Choose a reason for hiding this comment

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

looks safe to me, at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, anago runs the command above, which drops the relnotes into the given --release-bucket. The release-notify script downloads these notes when composing the email:

release/lib/releaselib.sh

Lines 1042 to 1052 in 920dcfb

local archive_root="gs://$bucket/archive/anago-$RELEASE_VERSION"
((FLAGS_nomock)) || mailto=$GCP_USER
mailto=${FLAGS_mailto:-$mailto}
# Announcement file is stored normally in WORKDIR, else check GCS.
if [[ -f "$announcement_file" ]]; then
announcement_text="$announcement_file"
else
announcement_file="$archive_root/announcement.html"
if ! $GSUTIL cp $announcement_file $announcement_text >/dev/null 2>&1; then

@dougm
Copy link
Member Author

dougm commented Sep 26, 2018

@ixdy @tpepper @neolit123 can you lift the 'hold' if you are ok with this?

@neolit123
Copy link
Member

change LGTM.

isn't there a way for having a dry-run - e.g. comment out sendmail calls etc and looking at the generated email?

@dougm
Copy link
Member Author

dougm commented Sep 26, 2018

I could do a mock build with my k/release PR branch, but that'll take quite a while. Otherwise, there's other stuff that needs to be faked to do a local test, such as release tars:

# @param release_tars - A directory containing tarballs to link to on GCS

Don't think it's worth the trouble in this case, since I can just copy-n-paste the 1 line that's been added :)

% echo "[Documentation](https://docs.k8s.io)"
[Documentation](https://docs.k8s.io)

Also note that the relnotes script is run by anago before the release-notify script would be run. The release-notify scripts downloads the file generated by relnotes to compose the email.

@neolit123
Copy link
Member

Don't think it's worth the trouble in this case, since I can just copy-n-paste the 1 line that's been added :)

ACK, i guess if the sendemail step is skipped or manual-ed then the email body can be amended before sending. also, this will verify if the generation step is correct.

/hold cancel
👉 🔘

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2018
@dougm
Copy link
Member Author

dougm commented Sep 26, 2018

I'll also do a full mock build when we have green light for the release and can double check the relnotes looks good before doing the --nomock release build.

@k8s-ci-robot k8s-ci-robot merged commit 6aac485 into kubernetes:master Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants