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

Clear extraneous CHANGELOG files on a new branch. #519

Merged

Conversation

david-mcmahon
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2018
anago Outdated
# CHANGELOG_FILE to the branch
clear_sync_changelogs () {
logecho -n "Checkout $RELEASE_BRANCH branch to make changes: "
logrun -s git checkout $RELEASE_BRANCH || return 1
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried that this branch switch is inside a function. You can't tell at the call site that this function will change the meaning of future git commands.

Can we either make the caller responsible for checking out the release branch, or save/restore the previous branch in the function?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2018
@david-mcmahon david-mcmahon changed the title WIP: Clear extraneous CHANGELOG files on a new branch. Clear extraneous CHANGELOG files on a new branch. Feb 28, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2018
@david-mcmahon
Copy link
Contributor Author

PTAL @enisoc. Thanks for the review!

anago Outdated
# describe'd as something other than the current tag on master
# If/when the generate-docs.sh issue is sorted out, this will become
# optional, though probably still useful anyway.
if [[ "$branch" =~ release- ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the $PARENT_BRANCH check above was causing docs to be generated only at the initial branch point (beta.0), not for subsequent releases. Is that right? If so, is there a reason we wouldn't need that check here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is why I keep sending you PRs. :) Yes, that is better to keep that logic here as well.

# optional, though probably still useful anyway.
if [[ "$PARENT_BRANCH" == "master" && "$branch" != "master" ]]; then
logecho -n "Remove any previous CHANGELOG-*.md files: "
logrun -s git rm -f CHANGELOG-*.md || return 1
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the CHANGELOG for the new release will already exist on master at this point, since it gets populated for alphas. Should we avoid deleting the one that matches the version in the branch name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that had got lost of the recent shuffle. Thanks. PTAL.

@enisoc
Copy link
Member

enisoc commented Mar 1, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants