#4163 | Speed-up CI by using published UI artifacts #4251

shubbham1215 · 2023-02-22T18:13:47Z

Which problem is this PR solving?

Resolves #4163

Short description of the changes

Attempt to download UI assets from GitHub release, fall back to regular manual build

codecov · 2023-02-22T18:31:42Z

Codecov Report

Patch coverage has no change and project coverage change: +0.02 🎉

Comparison is base (29d5688) 97.08% compared to head (af13a21) 97.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4251      +/-   ##
==========================================
+ Coverage   97.08%   97.10%   +0.02%     
==========================================
  Files         302      302              
  Lines       17685    17685              
==========================================
+ Hits        17169    17173       +4     
+ Misses        415      412       -3     
+ Partials      101      100       -1

Flag	Coverage Δ
unittests	`97.10% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugin/storage/integration/integration.go	`76.33% <0.00%> (+0.38%)`	⬆️
pkg/config/tlscfg/cert_watcher.go	`94.81% <0.00%> (+2.22%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

yurishkuro

Please explain how the change was tested

yurishkuro · 2023-02-22T18:32:42Z

Makefile

-	cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build
+	cd jaeger-ui
+
+	if git describe --exact-match --tags $(git rev-parse HEAD) >/dev/null 2>&1; then


Makefiles don't work with such multi-line scripts, unless you escape them. It may be easier to move all logic into scripts/rebuild-ui.sh

yurishkuro · 2023-02-22T18:37:07Z

Makefile

+		if curl --output /dev/null --silent --head --fail "$release_url"; then
+			# Download the file and unzip it into packages/build/
+
+			curl -L -o assets.tar.gz "$release_url"


I would prefer we use a temp file

tmpfile=$(mktemp) trap rm tmpfile EXIT

shubbham1215 · 2023-02-23T13:40:17Z

@yurishkuro This is still WIP and I need to rework on this. I'll take your suggestions and make the necessary changes.

shubbham1215 · 2023-03-01T18:12:34Z

@yurishkuro I have made some commits on this. I am not exactly sure how to get the version of the head commit. The command $ (git describe --exact-match --tags $(git rev-parse HEAD)) gives me the Hash associated with the latest commit but not the version. Can you look into the commit once?

yurishkuro · 2023-03-01T22:15:17Z

This seems to work:

# when on release tag
$ cd jaeger-ui
$ git describe --exact-match --tags
v1.27.3

# when on a commit without release tag
$ git checkout HEAD^1
$ git describe --exact-match --tags
fatal: no tag exactly matches '9b1ee69fa04482fa6e31e186f0ec790f21f28322'
# above also returns with an error code

yurishkuro · 2023-03-01T22:16:18Z

scripts/rebuild-ui.sh

+
+cd ../jaeger-ui
+
+if git describe --exact-match --tags $(git rev-parse HEAD) >/dev/null 2>&1; then


don't think you need $(git rev-parse HEAD), by default it gives you current commit

yurishkuro · 2023-03-01T22:16:39Z

scripts/rebuild-ui.sh

+if git describe --exact-match --tags $(git rev-parse HEAD) >/dev/null 2>&1; then
+
+    # Get the release version from the tag  
+    release_version = $(git describe --exact-match --tags $(git rev-parse HEAD)) 


no reason to run the same git command twice, read it into a var once

yurishkuro · 2023-03-01T22:17:49Z

scripts/rebuild-ui.sh

+
+
+    # Check if the corresponding UI release has the assets.tar.gz file uploaded
+    release_url="https://github.com/jaegertracing/jaeger-ui/releases/download/${release_version}/assets.tar.gz"


I wouldn't recommend using the output from git directly like that. Instead, match it against the release tag patter (vN.M.m) and only then use in the URL

yurishkuro · 2023-03-01T22:19:11Z

scripts/rebuild-ui.sh

+    if curl --output /dev/null --silent --head --fail "$release_url"; then
+        curl --location --output "$temp_file" "$release_url"
+        tar -zxvf "$temp_file" -C packages/jaeger_ui/build/
+        rm -f "$temp_file"


it's safer to use a trap right after you defined the var

scripts/rebuild-ui.sh

yurishkuro · 2023-03-01T22:21:17Z

scripts/rebuild-ui.sh

+
+    # Download the file to the temporary file and extract it into packages/build/
+    if curl --output /dev/null --silent --head --fail "$release_url"; then
+        curl --location --output "$temp_file" "$release_url"


Why do you need to check for --head first instead of trying to download right away? I assume it would still return an error exit code if the URL is invalid.

yurishkuro · 2023-03-01T22:22:05Z

scripts/rebuild-ui.sh

+        tar -zxvf "$temp_file" -C packages/jaeger_ui/build/
+        rm -f "$temp_file"
+
+    else


rather than having two duplicate else, just call exit 0 here. Something like:

tag=$(git describe ...) if tag matches pattern; then temp_file=$(mktemp) trap "rm -f ${temp_file}" EXIT if curl ... ; then untar exit 0 fi fi # do a regular full build yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build

shubbham1215 · 2023-03-04T18:34:40Z

I have added more commits.

1)Currently the $(git describe --exact-match --tags) command returns v1.27.3. Then it matches it with release tag patter(vN.M.m) and then tries the download it right away. As the assets file does not exist for this version, it fails with an exit code 0 and stops the execution. Do I need to make a change such that if it fails we perform a full build?

I tried to hardcode the tag variable with "v1.27.4" and then running the make run-all-in-one command. It was able to download the assets file and unzip it but later the build failed with the error
{"level":"panic","ts":1677953320.4700572,"caller":"app/static_handler.go:58","msg":"Could not create static assets handler","error":"cannot load index.html: cannot open index.html: open actual/index.html.gz: file does not exist"

Is this only because the current backend version is not compatible with the assets file that was downloaded or are there more changes required?

yurishkuro · 2023-03-04T20:50:40Z

you can bump your local Jaeger repo to v1.27.4 of the UI:

$ pwd
.../dev/jaegertracing/jaeger
$ cd jaeger-ui
$ git checkout v1.27.4

to restore it back

$ pwd
.../dev/jaegertracing/jaeger
$ git submodule update

yurishkuro · 2023-03-04T22:15:28Z

The error seems to suggest that your build did not place the assets in the right place where they could be picked up by the UI build.

yurishkuro · 2023-03-04T22:15:58Z

Makefile

@@ -214,7 +214,9 @@ jaeger-ui/packages/jaeger-ui/build/index.html:

 .PHONY: rebuild-ui
 rebuild-ui:
-	cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build
+	cd scripts && ./rebuild-ui.sh


Suggested change

cd scripts && ./rebuild-ui.sh

./scripts/rebuild-ui.sh

Yeah -C flag was creating packages/jaeger-ui/build/ again. Have fixed this with the latest commit

shubbham1215 · 2023-03-04T22:44:28Z

The build succeeds after we bump the UI version to v1.27.4. But fails for the version v1.27.3 as it tries to download right away and does not find the assets file and it exits after that. @yurishkuro Do I need to change this? The tests that have failed are because this issue.
Also please review Makefile changes again. I added one line in it to give permission for rebuild-ui.sh script.It works but not sure if it is the right way.

Signed-off-by: shubbham1215 <[email protected]>

yurishkuro · 2023-03-06T18:52:43Z

Makefile

+	chmod u+x scripts/rebuild-ui.sh && ./scripts/rebuild-ui.sh
+	


just call it with bash

Suggested change

chmod u+x scripts/rebuild-ui.sh && ./scripts/rebuild-ui.sh

bash ./scripts/rebuild-ui.sh

yurishkuro · 2023-03-06T19:01:15Z

I am going to merge #4282 soon so that v1.27.4 becomes the current version on main, then you can rebase and we'll see if the tests run

Signed-off-by: shubbham1215 <[email protected]>

yurishkuro · 2023-03-06T21:30:24Z

scripts/rebuild-ui.sh

+    trap "rm -f ${temp_file}" EXIT
+    release_url="https://github.com/jaegertracing/jaeger-ui/releases/download/${tag}/assets.tar.gz"
+
+     if curl -O "$release_url"; then


when I ran this, I got assets.tar.gz in the local dir. Why do you need to call curl twice?

I guess it was part of some earlier commit wherein I was first checking if the file exists instead of downloading right away. I have fixed this.

Signed-off-by: shubbham1215 <[email protected]>

scripts/rebuild-ui.sh

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-03-07T05:00:12Z

Thank you!

Which problem is this PR solving? Resolves #4163 Short description of the changes Issues raised by the previous pull request at #4251 --------- Signed-off-by: shubbham1215 <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

bobrik · 2023-06-26T22:14:54Z

This broke our internal build where we have some patches for jaeger-ui applied. Perhaps we should check for a clean git tree before downloading the jaeger-ui archive from GitHub.

The previous behavior resulted in the prebuild upstream package being downloaded, even if the local tree has any changes, whether they are commited or not. This broke our internal build that has some patches applied. See: * jaegertracing#4251 (comment) Let's use a more strict approach and only use the prebuild package if the tag is exact and does not contain any extra changes.

bobrik · 2023-06-26T22:47:56Z

I made #4553 to rectify this.

The previous behavior resulted in the prebuild upstream package being downloaded, even if the local tree has any changes, whether they are commited or not. This broke our internal build that has some patches applied. See: * jaegertracing#4251 (comment) Let's use a more strict approach and only use the prebuild package if the tag is exact and does not contain any extra changes. Signed-off-by: Ivan Babrou <[email protected]>

The previous behavior resulted in the prebuild upstream package being downloaded, even if the local tree has any changes, whether they are commited or not. This broke our internal build that has some patches applied. See: * #4251 (comment) Let's use a more strict approach and only use the prebuild package if the tag is exact and does not contain any extra changes. Signed-off-by: Ivan Babrou <[email protected]>

…racing#4553) The previous behavior resulted in the prebuild upstream package being downloaded, even if the local tree has any changes, whether they are commited or not. This broke our internal build that has some patches applied. See: * jaegertracing#4251 (comment) Let's use a more strict approach and only use the prebuild package if the tag is exact and does not contain any extra changes. Signed-off-by: Ivan Babrou <[email protected]> Signed-off-by: KevinSchneider <[email protected]>

…racing#4553) The previous behavior resulted in the prebuild upstream package being downloaded, even if the local tree has any changes, whether they are commited or not. This broke our internal build that has some patches applied. See: * jaegertracing#4251 (comment) Let's use a more strict approach and only use the prebuild package if the tag is exact and does not contain any extra changes. Signed-off-by: Ivan Babrou <[email protected]> Signed-off-by: Afzal Ansari <[email protected]>

shubbham1215 changed the title ~~Speed-up CI by using published UI artifacts https://github.com/jaegertracing/jaeger/issues/4163~~ #4163 | Speed-up CI by using published UI artifacts Feb 22, 2023

yurishkuro reviewed Feb 22, 2023

View reviewed changes

yurishkuro reviewed Mar 1, 2023

View reviewed changes

yurishkuro reviewed Mar 4, 2023

View reviewed changes

shubbham1215 force-pushed the softwarearchitecture branch from 7c8666b to 64592e1 Compare March 5, 2023 15:24

shubbham1215 closed this Mar 6, 2023

shubbham1215 force-pushed the softwarearchitecture branch from 64592e1 to 1d3bc1d Compare March 6, 2023 11:07

4163 | Speed-up CI by using published UI artifacts

1b66aa4

Signed-off-by: shubbham1215 <[email protected]>

shubbham1215 reopened this Mar 6, 2023

4163 | Speed-up CI by using published UI artifacts

c866872

Signed-off-by: shubbham1215 <[email protected]>

yurishkuro reviewed Mar 6, 2023

View reviewed changes

shubbham1215 and others added 2 commits March 7, 2023 00:49

4163 | Speed-up CI by using published UI artifacts

ef7439c

Signed-off-by: shubbham1215 <[email protected]>

Merge branch 'jaegertracing:main' into softwarearchitecture

33ee736

shubbham1215 marked this pull request as ready for review March 6, 2023 21:18

shubbham1215 requested a review from a team as a code owner March 6, 2023 21:18

shubbham1215 requested a review from yurishkuro March 6, 2023 21:18

yurishkuro reviewed Mar 6, 2023

View reviewed changes

4163 | Speed-up CI by using published UI artifacts

2503333

Signed-off-by: shubbham1215 <[email protected]>

yurishkuro reviewed Mar 6, 2023

View reviewed changes

scripts/rebuild-ui.sh Outdated Show resolved Hide resolved

Check for curl exit status

af13a21

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Mar 7, 2023

View reviewed changes

yurishkuro enabled auto-merge (squash) March 7, 2023 03:42

yurishkuro disabled auto-merge March 7, 2023 03:44

yurishkuro enabled auto-merge (squash) March 7, 2023 03:44

yurishkuro merged commit 579b24b into jaegertracing:main Mar 7, 2023

shubbham1215 deleted the softwarearchitecture branch March 7, 2023 10:48

yurishkuro mentioned this pull request Mar 9, 2023

Optimize all-in-one and Docker CI steps #1908

Closed

shubbham1215 restored the softwarearchitecture branch March 18, 2023 17:50

shubbham1215 mentioned this pull request Mar 19, 2023

#4163 | Speed-up CI by using published UI artifacts #4314

Merged

bobrik mentioned this pull request Jun 26, 2023

Rebuild jaeger-ui if the tree does not match any tag exactly #4553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4163 | Speed-up CI by using published UI artifacts #4251

#4163 | Speed-up CI by using published UI artifacts #4251

shubbham1215 commented Feb 22, 2023 •

edited by yurishkuro

Loading

codecov bot commented Feb 22, 2023 •

edited

Loading

yurishkuro left a comment

yurishkuro Feb 22, 2023

yurishkuro Feb 22, 2023

shubbham1215 commented Feb 23, 2023

shubbham1215 commented Mar 1, 2023 •

edited

Loading

yurishkuro commented Mar 1, 2023

yurishkuro Mar 1, 2023

yurishkuro Mar 1, 2023

yurishkuro Mar 1, 2023

yurishkuro Mar 1, 2023

yurishkuro Mar 1, 2023

yurishkuro Mar 1, 2023

shubbham1215 commented Mar 4, 2023

yurishkuro commented Mar 4, 2023

yurishkuro commented Mar 4, 2023

yurishkuro Mar 4, 2023

shubbham1215 Mar 4, 2023

shubbham1215 commented Mar 4, 2023 •

edited

Loading

yurishkuro Mar 6, 2023

yurishkuro commented Mar 6, 2023

yurishkuro Mar 6, 2023

shubbham1215 Mar 6, 2023

yurishkuro commented Mar 7, 2023

bobrik commented Jun 26, 2023

bobrik commented Jun 26, 2023


		cd ../jaeger-ui

		if git describe --exact-match --tags $(git rev-parse HEAD) >/dev/null 2>&1; then



		# Check if the corresponding UI release has the assets.tar.gz file uploaded
		release_url="https://github.com/jaegertracing/jaeger-ui/releases/download/${release_version}/assets.tar.gz"

	chmod u+x scripts/rebuild-ui.sh && ./scripts/rebuild-ui.sh

	bash ./scripts/rebuild-ui.sh

#4163 | Speed-up CI by using published UI artifacts #4251

#4163 | Speed-up CI by using published UI artifacts #4251

Conversation

shubbham1215 commented Feb 22, 2023 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Feb 22, 2023 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubbham1215 commented Feb 23, 2023

shubbham1215 commented Mar 1, 2023 • edited Loading

yurishkuro commented Mar 1, 2023

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

shubbham1215 commented Mar 4, 2023

yurishkuro commented Mar 4, 2023

yurishkuro commented Mar 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubbham1215 commented Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

yurishkuro commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Mar 7, 2023

bobrik commented Jun 26, 2023

bobrik commented Jun 26, 2023

shubbham1215 commented Feb 22, 2023 •

edited by yurishkuro

Loading

codecov bot commented Feb 22, 2023 •

edited

Loading

shubbham1215 commented Mar 1, 2023 •

edited

Loading

shubbham1215 commented Mar 4, 2023 •

edited

Loading