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 the Prow bump script smarter #4292

Closed

Conversation

stevekuznetsov
Copy link
Contributor

The script will now automatically detect which components need to be
bumped and determine the last time they were bumped to ensure that it is
not easy to forget to bump. Furthermore, this allows for a daily bump to
occur that will pick up all the changes that ocurred during that day.

Signed-off-by: Steve Kuznetsov [email protected]

/cc @spxtr @fejta @Kargakis @BenTheElder

What do y'all think about this? I'm consistently failing to bump the right thing -- or to bump at all! This should mean people can either add the bump as a presubmit hook or we can just bump once before deployment and catch all of the interim changes.

@k8s-ci-robot k8s-ci-robot added 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 Aug 31, 2017
The script will now automatically detect which components need to be
bumped and determine the last time they were bumped to ensure that it is
not easy to forget to bump. Furthermore, this allows for a daily bump to
occur that will pick up all the changes that ocurred during that day.

Signed-off-by: Steve Kuznetsov <[email protected]>
@spxtr
Copy link
Contributor

spxtr commented Aug 31, 2017

@rmmh original author.

What do y'all think about this? I'm consistently failing to bump the right thing -- or to bump at all! This should mean people can either add the bump as a presubmit hook or we can just bump once before deployment and catch all of the interim changes.

I like the idea of automatically bumping things daily, rather than having to do it as a presubmit. I would rather use bazel to create the docker images and push new versions instead of a shell script though.

@stevekuznetsov
Copy link
Contributor Author

I like the idea of automatically bumping things daily, rather than having to do it as a presubmit. I would rather use bazel to create the docker images and push new versions instead of a shell script though.

Absolutely -- I know Shell, so I took this route. If you can point me to how I would go about doing this with Bazel I will gladly use that path.

@spxtr
Copy link
Contributor

spxtr commented Aug 31, 2017

The first step is to build the prow docker images with bazel. That should be fairly straightforward, although there is a decision to be made about what base image to use (distroless and alpine both make sense). Then we use something like this to push the configs. It's still experimental, so probably that won't happen in the near future.

@spxtr
Copy link
Contributor

spxtr commented Aug 31, 2017

In the meantime, I'm open for suggestions to cutting back on the bump.sh rebase hell that we have these days.

@stevekuznetsov
Copy link
Contributor Author

Do you think this is viable as a short-term solution?

  1. merge this PR
  2. add a validation that any new commit NOT starting with that prefix up there should not change makefile version vars or deployment things
  3. run this script once a day or whenever you redeploy

@stevekuznetsov
Copy link
Contributor Author

I guess the question is -- do we expect to redeploy on every commit that touches Prow? if not, bumping should be on the list of things to do before a new release is pushed up to the GKE cluster running this stuff, not something that contributors should be doing.

@fejta
Copy link
Contributor

fejta commented Aug 31, 2017

I think we should kill the bump pattern. The version should be based on the commit. It should not be a function of the previous version.

I would like us to start using the YYYYMMDD-commitish pattern that we use everywhere else for test-infra.

Here's an example of how to use bazel to build and deploy images: https://github.com/kubernetes/test-infra/blob/master/experiment/commenter/BUILD

Agree with Joe that we should start decoupling decouple changing code from revving the version we deploy -- building a new version and deploying it should be a separate activity.

For updating what we deploy I would do it in two parts: update the version, deploy the merged version:

update version:
a) write a trivial script which replaces all tags in the yaml files with the YYYYMMDD-commitish pattern
b) build and pushes all prow images
c) runs a test to validate that all images exist
d) merge change

deploy version:

  1. sync to head
  2. deploy

Once prow has the ability to schedule jobs into multiple clusters (including ones that have more credentials) we could make update version a postsubmit prow job, and deploy a run_after success job chained on that.

@stevekuznetsov
Copy link
Contributor Author

@fejta that sounds good. Do you think as well that this is a good end state goal but may not be immediately achievable, so we could implement a quick solution to lessen rebase cost today?

@fejta
Copy link
Contributor

fejta commented Sep 1, 2017

I'm not a big fan of increasing the complexity of that script (I argued against checked it in in the first place because across time it attracts these sorts of improvements that increase complexity).

I see no reason to require people to bump when they check in code today. We just need to train @spxtr to stop asking people to bump prow when they change prow. Bumping prow should be a separate process done by people on the oncall rotation IMO.

That being said I won't hold back this merge if other people on test-infra want it.

@stevekuznetsov
Copy link
Contributor Author

@spxtr could just keep this script on his machine and use it to bump for his own purposes :)

@fejta
Copy link
Contributor

fejta commented Sep 1, 2017

Yeah if you and he want it that is reason enough to check it in 😁

@stevekuznetsov
Copy link
Contributor Author

/shrug

@stevekuznetsov
Copy link
Contributor Author

@spxtr can you make an executive decision about this please

@spxtr
Copy link
Contributor

spxtr commented Sep 18, 2017

I don't like this PR but at the same time I don't know the best way to solve what it attempts to solve. I think for now we can just skip bumping hook in most PRs. That lets us avoid the rebase issues, but we need to make sure to do a manual bump every now and then so we don't get far behind.

@stevekuznetsov
Copy link
Contributor Author

Right. Use this tool, use whatever you want, but if we stop asking contributors to bump and you have the tools to adequately bump everything when you need to, I think we are good.

/close

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants