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

Changelog checkin test: succeed for release PRs #658

Merged
merged 6 commits into from Apr 15, 2020

Conversation

dsherry
Copy link
Collaborator

@dsherry dsherry commented Apr 15, 2020

Fixes #542

Workaround for changelog checkin test for release PRs: If the 'future release' section is empty of entries with PR refs, mark check as successful

We don't need to update our release process after this PR. But we won't need to do a force-merge next time :)

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #658 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #658   +/-   ##
=======================================
  Coverage   98.97%   98.97%           
=======================================
  Files         136      136           
  Lines        4697     4697           
=======================================
  Hits         4649     4649           
  Misses         48       48           

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 5103778...1a8571b. Read the comment docs.

@dsherry dsherry force-pushed the ds_542_changelog_check_pass_for_release_prs branch from 057ccf6 to 7a02848 Compare Apr 15, 2020
@dsherry dsherry marked this pull request as ready for review Apr 15, 2020
@dsherry
Copy link
Collaborator Author

dsherry commented Apr 15, 2020

Some proof this change works:

  • Before I updated the changelog, the changelog test failed
  • After I updated the changelog, it succeeded
  • Simulating a release PR locally by deleting all PR entries from the future work section of the changelog and running the cmd produces the expected result of exit 0

Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Looks good to me - just one comment

@@ -190,8 +190,15 @@ jobs:
- image: busybox:latest
steps:
- checkout
- run: echo "${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}"
- run: cat docs/source/changelog.rst | grep ":pr:\`${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}\`"
- run: | # debugging info
Copy link
Contributor

@jeremyliweishih jeremyliweishih Apr 15, 2020

Choose a reason for hiding this comment

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

Was this left intentionally?

Copy link
Collaborator Author

@dsherry dsherry Apr 15, 2020

Choose a reason for hiding this comment

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

Yeah, helpful to have if stuff goes wrong. The comment isn't super necessary but I figured it can't hurt

Copy link
Contributor

@jeremyliweishih jeremyliweishih Apr 15, 2020

Choose a reason for hiding this comment

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

I agree! Just wanted to make sure

Copy link
Contributor

@angela97lin angela97lin left a comment

Just a question to make sure I understand but otherwise LGTM! 🛥️

- run: echo "${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}"
- run: cat docs/source/changelog.rst | grep ":pr:\`${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}\`"
- run: | # debugging info
echo $(sed '/^\*\*v/q' docs/source/changelog.rst | grep -iE ":pr:" | wc -l)
Copy link
Contributor

@angela97lin angela97lin Apr 15, 2020

Choose a reason for hiding this comment

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

Oo just so I understand properly, is this just printing for our sake? Not functionally necessary?

Copy link
Collaborator Author

@dsherry dsherry Apr 15, 2020

Choose a reason for hiding this comment

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

Yeah, just for debug if anything goes wrong later

.circleci/config.yml Show resolved Hide resolved
… release' section is empty of entries with PR refs, mark check as successful
@dsherry dsherry force-pushed the ds_542_changelog_check_pass_for_release_prs branch from 48573cf to b91450f Compare Apr 15, 2020
@kmax12
Copy link
Contributor

kmax12 commented Apr 15, 2020

@dsherry does this work for the first PR someone makes on a new release? it seems like if the person forgot to add a PR line it wouldn't error in that case since there would be no lines in the future changes

@dsherry
Copy link
Collaborator Author

dsherry commented Apr 15, 2020

@kmax12 good point, that is true! I guess that's not ideal. The first PR to merge after a release PR goes in will likely be one which was queued up beforehand, meaning the check would have been red. But why make our process more error-prone.

Your saying that made me realize: a more foolproof alternative would be to check the CIRCLE_BRANCH env var and pass the check if that matches "release_v[0-9\.]+", and then always have release PRs use that branching strategy. I think I prefer that approach. Will spend 30min on that.

echo "${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}"
- run: |
# for release PRs: if the "future release" segment of the changelog contains no PR refs (sed/grep/wc returns no lines), mark as successful
if [[ "${CIRCLE_BRANCH}" =~ "^release_v[0-9\.]+" ]]; then
Copy link
Collaborator Author

@dsherry dsherry Apr 15, 2020

Choose a reason for hiding this comment

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

Ok @jeremyliweishih @angela97lin : RE @kmax12 suggestion I updated this impl to require release branches to be of the form release_vX.X.X (the regex is a bit looser than that but I think that's fine). LMK if you see any issues with that and I can do an update in another PR.

- run: echo "${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}"
- run: cat docs/source/changelog.rst | grep ":pr:\`${CIRCLE_PULL_REQUEST##https://github.com/FeatureLabs/evalml/pull/}\`"
- run: | # debugging info
[[ "${CIRCLE_BRANCH}" =~ "^release_v[0-9\.]+" ]] && echo This is not a release PR || echo This is a release PR
Copy link
Collaborator Author

@dsherry dsherry Apr 15, 2020

Choose a reason for hiding this comment

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

Used control operators to make this easy to read

@dsherry dsherry force-pushed the ds_542_changelog_check_pass_for_release_prs branch from 727f995 to 81c6197 Compare Apr 15, 2020
@dsherry
Copy link
Collaborator Author

dsherry commented Apr 15, 2020

Aw man, took 35min, but, got it now 😂 turns out circleci uses sh by default, not bash. That's a headache for another day 🤷‍♂️

@dsherry dsherry merged commit 6e5553b into master Apr 15, 2020
2 checks passed
@dsherry dsherry deleted the ds_542_changelog_check_pass_for_release_prs branch Apr 15, 2020
@dsherry
Copy link
Collaborator Author

dsherry commented Apr 28, 2020

Random but I'm pleased to note this worked for the 0.9.0 release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip release PRs in changelog check
4 participants