Added a deploy stage to automate the deployment to the official images repo

[LaurentGoderre]

Closes #431

[LaurentGoderre]

In support of #670

[LaurentGoderre]

Should probably be mergedcombined with #431

[LaurentGoderre]

Can we close #431 in favor of this? I attributed @hnryjms in this PR

[LaurentGoderre]

And here's what the result of the script is: docker-library/official-images#4175

It should be the Node Bot but I use my own account for the test because we don't have a token for the bot yet.

[SimenB]

This looks really good! Loving it

[chorrell]

Nice!

RE: #431 I think maybe close that after this PR gets merged

[SimenB]

Added that to the OP, then GitHub will close it automatically linking to this PR once this is merged

[LaurentGoderre]

Hmmm, the script needs some tweak I think

https://travis-ci.org/nodejs/docker-node/jobs/358992748

[LaurentGoderre]

The new payload was failing so I fixed that. It also revealed that I should be handling the response of the API call. There's also new TODOs.

[LaurentGoderre]

However docker-library/official-images#4183 was created with the updated script.

[LaurentGoderre]

@SimenB @chorrell @PeterDaveHello the script changed quite a bit since the last review. Can you check it again?

[LaurentGoderre]

My mastery of shell scripting exponentially grew by doing this pr!

[PeterDaveHello]

I'm not sure if it's good to "dismiss" the review if things changed, request review again maybe more properly :)

[PeterDaveHello]

I suggest having the review from @chorrell and @SimenB before merging.

[LaurentGoderre]

@PeterDaveHello i think I have to dismiss the review to request another review. Im not really dismissing as much as marking your requested changes as resolved. Is that not the process?

[LaurentGoderre]

Anyways, this isn't quite ready because the secure token isn't working yet.

[LaurentGoderre]

@PeterDaveHello do you know what that onbuild error is?

[LaurentGoderre]

@SimenB @chorrell can I get a final review for this, it's ready to go.

[PeterDaveHello]

hmmm, the problem seems to be consistent among multi rounds on CI ... trying to figure it out ...

[chorrell]

The image that onbuild depends on (FROM) doesn't exist:

$ docker pull node:9.11.1
Error response from daemon: manifest for node:9.11.1 not found

[chorrell]

The PR is still open:

docker-library/official-images#4213

[LaurentGoderre]

@chorrell @SimenB can we merge this?

[chorrell]

I think we need to talk more about the consequences of implementing this and what it means for image updates. I’m not clear on a few things. For instance:

• What happens when an image build fails? Will it still trigger a deploy?
• If a build failure can be ignored (like a timeout) and a deploy PR is cancelled, how do we handle it?
• Is there a way to omit the deploy PR for some changes?
• We’ve been trying to be strict about image updates. For instance, we try to bundle changes with a Node.js update so users call roll back or pin to a specific version. How will this affect that?

Also, I was wondering how we can track deploy PRs in this repo so it’s obvious to users (and us) that a PR to the docker hub has been made.

[chorrell]

Also what about security updates? Our current process is to highlight this docker hub prs by putting [security] in the title.

[LaurentGoderre]

Happy to explain the process

A deploy is only triggered if the build succeeds and one of the images was changed (changes to the build itself or documentation, or even templates if the images themselves don't change) will not trigger a deploy.

If a build fails and can be ignored, then the upstream PR will have to manually be created, though we can perhaps adapt the script in the future to allow it to be ran locally.

When a PR is created that PR contains a link to the push commit that initiated it. Perhaps we add to the script to report back on the PR in this repo as well (though I would personally prefer to do this after we land this because it's already a big PR)

The PR created upstream uses the title of the commit that initiated the build. As long as the commit message has [security] in the title (preferably at the start), then the PR will reflect that.

[SimenB]

A message posted back in the merged PR with link to the upstream one sounds great, but I agree it can come in a separate PR. Especially on timeouts so we're notified

[chorrell]

@LaurentGoderre Thanks, that answers all my questions :)

I agree that adding something to the script to report back on the PR in this repo is something that can be done in another PR.

Going forward we'll need to ensure our commit titles are always meaningful (e.g., "Update node.js v9.x to v9.11.1") and make sure we use [security] to preface any security updates..

Nice work!

[hnryjms]

You all are amazing 🎉 Apologies on not being able to carry my original PR into the end.

[LaurentGoderre]

@hnryjms I attributed you in the commit since you did half of the original work!