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

Make PRs, *don't* push directly to master during release process #13

Closed
lavalamp opened this issue Jun 15, 2016 · 13 comments
Closed

Make PRs, *don't* push directly to master during release process #13

lavalamp opened this issue Jun 15, 2016 · 13 comments
Assignees
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/P1 sig/release Categorizes an issue or PR as relevant to SIG Release.

Comments

@lavalamp
Copy link
Member

It really confuses the build cop and could interfere with what they're doing at the moment.

@david-mcmahon
Copy link
Contributor

david-mcmahon commented Jun 15, 2016

We talked offline about this some:

  • infra errors/gotchas by their nature have more impact than bugs in the product
  • Release velocity would be significantly impacted by inserting unnecessary roadblocks into the process
  • Non-atomic (some changes to branch now and some to master later) changes could introduce unforeseen issues
  • Continuous integration / testing of release is on the releng roadmap. This will help mitigate most issues
  • In general we should vet and test code that that does the Right Thing and then allow those automated process to do its job (with non-human checks)
  • Inserting humans into processes more often causes delays and non-determinism. Those are not things you want in a release cycle
  • Solve problems and enforce policy through code, not (more) doc or tribal knowledge.
  • Use this issue to identify short and long term resolutions here

@lavalamp
Copy link
Member Author

And as I said:

  • Build cop needs to be aware this script is being run and that it is going to push commits to master. The human running the script should not run the script if the build cop is putting out a fire.
  • The submit queue's current test is technically invalidated by this push to master.

@david-mcmahon
Copy link
Contributor

david-mcmahon commented Jun 15, 2016

Identifying the things pushed to master during a release

How do those direct pushes affect the tree (adversely) and how can we notify/mitigate (through code)?

@david-mcmahon
Copy link
Contributor

Concerns over the release process and pushing to master should be discussed in this issue.
These actions are currently expressed in code and should be addressed there as well.

Releases happen at least every 2 weeks and push tags and minimal changes to up the master as part of it. Lets get any concerns addressed and fixed in the code.

This comment points to the places were master is updated. Please take a look and make suggestions, either here or in the form of a PR.

@david-mcmahon
Copy link
Contributor

Note: There are releases coming up at least one a week for the next few weeks. If we want to do something to address this, let's do it sooner than later.

@lavalamp
Copy link
Member Author

Unfortunately I really don't have time to wrap my head around that bash-- I'm doing too many things.

@fabioy is the next build cop. Fabio, do you have time to look at this?

@david-mcmahon more thoughts:

  • I glanced at the code and didn't see any verify scripts getting called, but I probably missed it. I also don't trust running the scripts locally to be a good proxy for whether they will fail in jenkins and block the queue for everyone.
  • I don't think it's right that the release script makes changes which don't get the same level of checking as other changes. I take your point that the release script always needs to work. But I am not convinced that sufficient safeguards are taken against bitrot-- changes to the repo that invalidate the release script's assumptions.
  • I do think we should make an exception for making the tag. That should be in its own commit, and I think that should be safe to push and should never screw anything up (knock on wood)
  • However I think the safest thing is for the other things the script does to get dumped into a P0 retest-not-required PR.

@fabioy
Copy link

fabioy commented Jun 17, 2016

@lavalamp Yeah, I can take a look at this. I'll try to get the context from this issue, else I'll ping David... :)

@david-mcmahon
Copy link
Contributor

david-mcmahon commented Jun 17, 2016

Let's look more closely at each commit to master:

  • mungedocs - Dependent .go sources updated and update-munge-docs.sh run to handle the generated files. NOTE: Only necessary for new branches
  • update the base.go file - gitMajor, gitMinor values tied closely to the tag that is pushed at the same time. These should be atomic changes.
  • CHANGELOG.md - The release notes published at the time of the release. I've gotten pings and email from people in the past before this process was streamlined as to "Why aren't the release notes available yet". We don't want to go back to those dark times. Release notes published at release time is correct.
  • tags - Self explanatory.

These are small release (infra) related changes that accompany the state of the tree through a release.

The right direction here might be looking at how to avoid changing files on master at all (except for tags):

  • The whole mungedocs thing seems silly to me and I know that's in some kind of flux, but maybe we can do away with changes to master there in the future.
  • There is a P3 Issue to get rid of base.go changes, but I don't know how feasible that is. Brian said at the time at least it was not possible.
  • CHANGELOG.md - We probably want to keep this one. update-munge.docs.sh is the only dependency that I'm aware of.

Edit: the base.go change is only on the branch and mungedocs is only at new branch time (every few months)

@david-mcmahon
Copy link
Contributor

david-mcmahon commented Jun 17, 2016

Another thought on the CHANGELOG.md. While it's certainly customary and desirable to have a CHANGELOG in the root directory that reflects release notes/changes, we could go back to publishing those solely on the github releases page or in another repo altogether (kubernetes.github.io? @johndmulhausen)
My concerns there are discoverability, inconvenience, release note location/process churn and related community blowback. Then again, maybe noone cares that much.

@lavalamp
Copy link
Member Author

I can buy that the base.go change is necessary and safe in addition to the
tags change.

.md file changes are scarier on account of our mungers...

On Fri, Jun 17, 2016 at 4:25 PM, David McMahon notifications@github.com
wrote:

Another thought on the CHANGELOG.md. While it's certainly customary and
desirable to have a CHANGELOG in the root directory that reflects release
notes/changes, we could go back to publishing those solely on the github
releases page or in another repo altogether (kubernetes.github.io?
@johndmulhausen https://github.com/johndmulhausen)
My concerns there are discoverability, inconvenience, release note
public/process churn and related community blowback. Then again, maybe
noone cares that much.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#13 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAnglqq6KLin5sLdF1akB_J3cCGjfEmEks5qMyzegaJpZM4I2BXt
.

@fejta-bot
Copy link

Issues go stale after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2017
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 15, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

marpaia pushed a commit to marpaia/release that referenced this issue Feb 21, 2019
@justaugustus justaugustus added sig/release Categorizes an issue or PR as relevant to SIG Release. area/release-eng Issues or PRs related to the Release Engineering subproject labels Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/P1 sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
Development

No branches or pull requests

6 participants