Release automation improvements #10707

justinas · 2022-03-01T19:43:04Z

This PR tries to address https://github.com/gravitational/cloud/issues/1199 and https://github.com/gravitational/cloud/issues/1272 .

There are several changes included under this single PR, in order to cut down on the amount of backports and merge conflicts. Each commit includes an extended message where I tried to include some background information. Please do not hesitate to raise any questions or concerns.

This should be the penultimate batch of changes concerning integration with the release service. After this is merged, if no new issues occur, we shall follow up with another PR that switches to the production endpoint and removes failure:ignore from these steps.

Tag pipeline test: https://drone.platform.teleport.sh/gravitational/teleport/11278

russjones · 2022-03-08T14:59:57Z

@justinas Just a heads up, we might be slow on reviews this week due to focusing on getting Teleport 9 out, but we'll try and get to this.

@r0mant @fheinecke @tcsc Can you review?

tcsc

Looks OK to me, modulo a few questions. You will want to get someone with better bash than me to review it before submission, though ;-)

tcsc · 2022-03-10T00:34:03Z

dronegen/common.go

@@ -86,6 +87,73 @@ type buildType struct {
 	windowsUnsigned bool
 }

+// Description provides a human-facing description of the artifact


Would be nice to see an example of the output in the docstring

tcsc · 2022-03-10T00:41:21Z

dronegen/common.go

+	result = os
+
+	if b.os != "darwin" {
+		// arch is implicit for i386/amd64


Is the empty arch for a MacOS intel build a side-effect of how the buildType value was originally constructed, or is there a deeper reason?

Also, is it worth making explicit in the human-readable description, given that we're in the middle of the whole M1 transition, and I can picture people on both sides of that transition blindly selecting the first MacOS version they see that doesn't explicitly exclude them.

Mostly this was an attempt to keep the descriptions as close to the current ones. I do agree that it makes sense to be more explicit in some cases.

For darwin, bitness is not relevant, but we could add an arch specifier, e.g.:

MacOS Intel

MacOS Apple Silicon

tcsc · 2022-03-10T00:53:30Z

dronegen/tag.go

-  curl $CREDENTIALS --fail -o /dev/null -F description="TODO" -F os="%s" -F arch="%s" -F "file=@$file" -F "sha256=$shasum" -F "releaseId=$product@$VERSION" "$RELEASES_HOST/assets";
+  shasum="$(cat "$file.sha256" | cut -d ' ' -f 1)"
+
+  curl $CREDENTIALS --fail -o /dev/null -F description="%s" -F os="%s" -F arch="%s" -F "file=@$file" -F "sha256=$shasum" "$RELEASES_HOST/assets";


Just a thought - the variables being substituted here are so far away from the site that they're being used that it might be worth using the explicit argument indices, just to highlight what's going on.

Suggested change

curl $CREDENTIALS --fail -o /dev/null -F description="%s" -F os="%s" -F arch="%s" -F "file=@$file" -F "sha256=$shasum" "$RELEASES_HOST/assets";

curl $CREDENTIALS --fail -o /dev/null -F description="%[1]s" -F os="%[2]s" -F arch="%[3]s" -F "file=@$file" -F "sha256=$shasum" "$RELEASES_HOST/assets";

or, if you wanted to go bananas, you could use a text/template to make it a bit more flexible, but that is probably overkill - and you also may have escaping issues with the template language and a shell script.

Who receives that POST, btw?

This is all handled by "releaseserver" https://github.com/gravitational/cloud/tree/master/pkg/releaseserver . This request in particular handled here https://github.com/gravitational/cloud/blob/1e466d0df1a3aef70690e99a0e3cabb0e8c5b602/pkg/releaseserver/handler/handler.go#L36-L37

tcsc · 2022-03-10T00:54:35Z

dronegen/tag.go

@@ -44,7 +45,12 @@ func tagCheckoutCommands(fips bool) []string {
 		// create necessary directories
 		`mkdir -p /go/cache /go/artifacts`,
 		// set version
-		`if [[ "${DRONE_TAG}" != "" ]]; then echo "${DRONE_TAG##v}" > /go/.version.txt; else egrep ^VERSION Makefile | cut -d= -f2 > /go/.version.txt; fi; cat /go/.version.txt`,
+		`VERSION=$(egrep ^VERSION Makefile | cut -d= -f2)
+if [ "$$VERSION" != "${DRONE_TAG##v}" ]; then


This screams of a bad experience.

I am open to alternative approaches, but this does prevent a real issue we have already encountered and it was the solution proposed. https://github.com/gravitational/cloud/issues/1272#issuecomment-1034476738

Instead of using even more bash, can we please extend our version check tool to support this and use it instead? It's already used in some other places e.g. checking for pre-release.

cmd/check does not seem to exist in branch/v7, and does not seem to be fully backported (up-to-date) in branch/v8. We could try to backport it everywhere, perhaps.

cmd/check does not seem to exist in branch/v7, and does not seem to be fully backported (up-to-date) in branch/v8. We could try to backport it everywhere, perhaps.

I suspect #10436 is where the port was not fully backported.

I chose not to backport the docker image publishing fixes (for #9494), as that drone pipeline is only ever run off master.

If it helps these efforts, I can backport the rest of #10295 to v8 and v7. Let me know.

tcsc · 2022-03-10T00:57:00Z

dronegen/tag.go

@@ -457,7 +478,7 @@ func tagPackagePipeline(packageType string, b buildType) pipeline {
 		{
 			Name:     "Register artifacts",
 			Image:    "docker",
-			Commands: tagCreateReleaseAssetCommands(b),
+			Commands: tagCreateReleaseAssetCommands(b, strings.ToUpper(packageType), nil),


Is there something wrong with the indentation around here?

I'm not sure I understand the comment. The indentation seems to be unchanged on this line?

tcsc · 2022-03-10T01:04:57Z

dronegen/tag.go

+
+  for product in $products; do
+    status_code=$(curl $CREDENTIALS -o "$WORKSPACE_DIR/curl_out.txt" -w "%%{http_code}" -F "product=$product" -F "version=$VERSION" -F notesMd="# Teleport $VERSION" -F status=draft "$RELEASES_HOST/releases")
+    if [ $status_code -ne 200 ] && [ $status_code -ne 409 ]; then


Why is status 409 special here?

Drone runs independent package pipelines in parallel. As we do not have a step "before building any packages" where we could create the release, each pipeline tries to first create a release [email protected]. In case it has already been created by another pipeline, 409 is raised, as the release is uniquely identified by (product, version). If another pipeline has already created the release, it is fine to continue and attach the assets to it.

r0mant · 2022-03-23T23:05:31Z

dronegen/common.go

+	case "windows":
+		os = "Windows"
+	default:
+		panic("unhandled OS")


I don't know if we want to panic. Can we return a proper error and include the actual b.os value in the error message for troubleshooting?

We could return an error, but that introduces a lot of error propagation up the call stack (unless at some point we still panic() or log.Fatal()):

(*buildType).Description()

tagCreateReleaseAssetCommands()

tagPipeline()

tagPipelines()

main()

All for what is currently the sole fallible step in generating the YAML itself.

I agree with improving the error message.

r0mant · 2022-03-23T23:05:45Z

dronegen/common.go

+		bitness = 32
+
+	default:
+		panic("unhandled arch")


Same comment as above.

r0mant · 2022-03-23T23:11:20Z

dronegen/tag.go

@@ -44,7 +45,12 @@ func tagCheckoutCommands(fips bool) []string {
 		// create necessary directories
 		`mkdir -p /go/cache /go/artifacts`,
 		// set version
-		`if [[ "${DRONE_TAG}" != "" ]]; then echo "${DRONE_TAG##v}" > /go/.version.txt; else egrep ^VERSION Makefile | cut -d= -f2 > /go/.version.txt; fi; cat /go/.version.txt`,
+		`VERSION=$(egrep ^VERSION Makefile | cut -d= -f2)
+if [ "$$VERSION" != "${DRONE_TAG##v}" ]; then


Instead of using even more bash, can we please extend our version check tool to support this and use it instead? It's already used in some other places e.g. checking for pre-release.

r0mant · 2022-03-23T23:18:24Z

dronegen/tag.go

-    echo "curl HTTP status: $status_code"
-    cat $WORKSPACE_DIR/curl_out.txt
-    exit 1
+  name="$(basename "$file" | sed -E 's/(-|_)v?[0-9].*$//')" # extract part before -vX.Y.Z


Any chance all of this can be implemented in Go? I'm having a hard time following the logic here. I think we should be reducing amount of bash code we have, not adding it.

This is a good idea, we have recently created a basic Go client (see https://github.com/gravitational/cloud/pull/1433), but currently it implements a different subset of operations on this API. I would definitely like to move as much logic as possible to the Go client in the long run (it would also have the benefit of reducing backports to Teleport version branches), but I'm not sure if it is doable in Q1 (by the end of which we would like for CDN to go live).

Created an issue so this does not get lost: https://github.com/gravitational/cloud/issues/1504

justinas · 2022-04-01T14:22:16Z

Some merge conflicts arose in .drone.yml (as they usually do). Will solve those and re-run dronegen after getting the approval.

r0mant

@justinas I'm gonna approve cause I don't want to block you but can you please make sure that https://github.com/gravitational/cloud/issues/1504 does not get lost and is actually addressed? For all future changes to the pipeline, unless it's something trivial, please prefer writing tools in Go as well. cc @wadells

* [v8] Release pipeline improvements (#10707) * fixup steps * re-sign .drone.yml

github-actions bot requested review from espadolini and timothyb89 March 1, 2022 19:43

pierrebeaucamp added the cloud Cloud label Mar 1, 2022

russjones requested review from r0mant, fheinecke and tcsc and removed request for espadolini and timothyb89 March 2, 2022 00:49

tcsc approved these changes Mar 10, 2022

View reviewed changes

r0mant requested changes Mar 23, 2022

View reviewed changes

r0mant approved these changes Apr 1, 2022

View reviewed changes

justinas force-pushed the justinas/release-improvements branch from d3aff9d to 5f726f0 Compare April 4, 2022 12:51

Release pipeline improvements

d34b8ad

justinas force-pushed the justinas/release-improvements branch from d4cd99e to d34b8ad Compare April 5, 2022 15:43

Merge branch 'master' into justinas/release-improvements

fe84509

justinas merged commit b24b561 into master Apr 6, 2022

justinas deleted the justinas/release-improvements branch April 6, 2022 13:35

justinas added a commit that referenced this pull request Apr 8, 2022

[v8] Release pipeline improvements (#10707)

7ee1425

justinas added a commit that referenced this pull request Apr 8, 2022

[v7] Release pipeline improvements (#10707)

712133c

justinas added a commit that referenced this pull request Apr 8, 2022

[v9] Release pipeline improvements (#10707)

32a33da

justinas added a commit that referenced this pull request Apr 8, 2022

[v8] Backport #10707 (#11831)

8744719

* [v8] Release pipeline improvements (#10707) * fixup steps * re-sign .drone.yml

justinas added a commit that referenced this pull request Apr 11, 2022

[v7] Release pipeline improvements (#10707) (#11832)

814cfa8

justinas added a commit that referenced this pull request Apr 11, 2022

[v9] Release pipeline improvements (#10707)

ea89fea

justinas added a commit that referenced this pull request Apr 11, 2022

[v9] Release pipeline improvements (#10707) (#11833)

4088375

webvictim mentioned this pull request Apr 19, 2022

opened in error #12068

Closed

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

justinas added a commit that referenced this pull request Jul 26, 2022

[v6.2] Release automation improvements (#10707)

cf4d206

justinas mentioned this pull request Jul 27, 2022

[v6.2] Backport #10707, #13946 #14929

Closed

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release automation improvements #10707

Release automation improvements #10707

justinas commented Mar 1, 2022 •

edited

Loading

russjones commented Mar 8, 2022

tcsc left a comment •

edited

Loading

tcsc Mar 10, 2022

tcsc Mar 10, 2022

justinas Mar 10, 2022

tcsc Mar 10, 2022

tcsc Mar 10, 2022

justinas Mar 10, 2022

tcsc Mar 10, 2022

justinas Mar 16, 2022

r0mant Mar 23, 2022

justinas Mar 24, 2022

wadells Apr 1, 2022

tcsc Mar 10, 2022

justinas Mar 10, 2022

tcsc Mar 10, 2022

justinas Mar 10, 2022

r0mant Mar 23, 2022

justinas Mar 24, 2022

r0mant Mar 23, 2022

r0mant Mar 23, 2022

r0mant Mar 23, 2022

justinas Mar 24, 2022

justinas Apr 1, 2022

justinas commented Apr 1, 2022

r0mant left a comment

	curl $CREDENTIALS --fail -o /dev/null -F description="%s" -F os="%s" -F arch="%s" -F "file=@$file" -F "sha256=$shasum" "$RELEASES_HOST/assets";
	curl $CREDENTIALS --fail -o /dev/null -F description="%[1]s" -F os="%[2]s" -F arch="%[3]s" -F "file=@$file" -F "sha256=$shasum" "$RELEASES_HOST/assets";

Release automation improvements #10707

Release automation improvements #10707

Conversation

justinas commented Mar 1, 2022 • edited Loading

russjones commented Mar 8, 2022

tcsc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinas commented Apr 1, 2022

r0mant left a comment

Choose a reason for hiding this comment

justinas commented Mar 1, 2022 •

edited

Loading

tcsc left a comment •

edited

Loading