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

Fix artifact registration in Releases API for Teleport Connect #13946

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

justinas
Copy link
Contributor

@justinas justinas commented Jun 28, 2022

Teleport Connect-X.Y.Z.dmg has recently been added to CI builds. This change fixes a couple of things to support it:

  • Support artifacts with spaces in the name
  • Assign Teleport Connect to both teleport (OSS) and teleport-ent (Enterprise) releases - same logic as for tsh packages.
  • Add custom artifact description for Teleport Connect (aligned with what Houston currently has).

This is undoubtedly somewhat hacky, but I hope to re-code this logic in a nicer way with the move to Go (https://github.com/gravitational/cloud/issues/1504). This change only concerns master and branch/v10, but I will try to port to older branches as well for consistency (also minimizes merge conflicts down the line).

All Dronegen changes auto-generated.

@github-actions github-actions bot requested review from LKozlowski and r0mant June 28, 2022 20:15
@@ -375,7 +375,7 @@ for file in $(find . -type f ! -iname '*.sha256' ! -iname '*-unsigned.zip*'); do
cat $WORKSPACE_DIR/curl_out.txt
exit 1
fi
curl $CREDENTIALS --fail -o /dev/null -X PUT "$RELEASES_HOST/releases/$product@$VERSION/assets/$(basename $file)"
curl $CREDENTIALS --fail -o /dev/null -X PUT "$RELEASES_HOST/releases/$product@$VERSION/assets/$(basename "$file" | sed 's/ /%%20/g')"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curl encodes data (body or url query), but not URLs, it expects them pre-encoded.

@justinas justinas force-pushed the justinas/rlz-teleterm branch from d8eeb13 to 077fd72 Compare July 20, 2022 16:06
@justinas justinas enabled auto-merge (squash) July 20, 2022 16:10
@justinas justinas disabled auto-merge July 20, 2022 16:10
@justinas justinas force-pushed the justinas/rlz-teleterm branch from 077fd72 to f09a00c Compare July 21, 2022 17:41
@r0mant r0mant requested review from tcsc and removed request for r0mant July 25, 2022 20:17
@@ -1228,7 +1231,7 @@ steps:
cat $WORKSPACE_DIR/curl_out.txt
exit 1
fi
curl $CREDENTIALS --fail -o /dev/null -X PUT "$RELEASES_HOST/releases/$product@$VERSION/assets/$(basename $file)"
curl $CREDENTIALS --fail -o /dev/null -X PUT "$RELEASES_HOST/releases/$product@$VERSION/assets/$(basename "$file" | sed 's/ /%20/g')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more general way to urlencode the filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find an easily available standalone utility to urlencode, aside from snippets like this https://stackoverflow.com/a/10660730/1264974

IMO it's fine for now, as our filenames are pretty uniform, and Teleport Connect is the first thing that needs to be urlencoded (all others roughly fall into the [A-z0-9._-] set)

@@ -1206,20 +1206,23 @@ steps:
- which curl || apk add --no-cache curl
- |-
cd "$WORKSPACE_DIR/go/artifacts"
for file in $(find . -type f ! -iname '*.sha256' ! -iname '*-unsigned.zip*'); do
find . -type f ! -iname '*.sha256' ! -iname '*-unsigned.zip*' | while read -r file; do
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between for file in ... vs find ... | while read ... - asking for a bash deficient person (i.e. me)

Copy link
Contributor Author

@justinas justinas Jul 26, 2022

Choose a reason for hiding this comment

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

Not an expert either, but if I remember correctly: for will split by IFS, which by default includes spaces. One could in theory use something like IFS=$'\n' for ... to only split elements by newline, but I think this did not work as easily with alpine's shell (which does not have some of bash's stuff).

read X Y Z will read a line, and still split it to words (by spaces), but if you have more output variables than words, the remainder will end up in the last variable. E.g. with input foo bar baz quux, read X Y Z will end up in Z="baz quux". Thus, with only one output variable (read X), no splitting is performed, and in effect the whole line ends up in X unsplitted.

@justinas justinas enabled auto-merge (squash) July 26, 2022 12:07
@justinas justinas merged commit 08dcdcd into master Jul 26, 2022
justinas added a commit that referenced this pull request Jul 26, 2022
justinas added a commit that referenced this pull request Jul 26, 2022
justinas added a commit that referenced this pull request Jul 26, 2022
justinas added a commit that referenced this pull request Jul 26, 2022
justinas added a commit that referenced this pull request Jul 27, 2022
justinas added a commit that referenced this pull request Jul 27, 2022
justinas added a commit that referenced this pull request Jul 27, 2022
@zmb3 zmb3 deleted the justinas/rlz-teleterm branch September 9, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants