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

Prepare release v1.43.0 #4298

Merged
merged 6 commits into from
Mar 15, 2023
Merged

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Mar 13, 2023

TODOs

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (87098d1) 97.10% compared to head (9fc3d3e) 97.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4298      +/-   ##
==========================================
- Coverage   97.10%   97.09%   -0.01%     
==========================================
  Files         300      300              
  Lines       17659    17659              
==========================================
- Hits        17147    17146       -1     
- Misses        412      413       +1     
  Partials      100      100              
Flag Coverage Δ
unittests 97.09% <ø> (-0.01%) ⬇️

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

see 1 file with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pavolloffay pavolloffay marked this pull request as ready for review March 15, 2023 09:35
@pavolloffay pavolloffay requested a review from a team as a code owner March 15, 2023 09:35
@pavolloffay pavolloffay requested a review from vprithvi March 15, 2023 09:35
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
CHANGELOG.md Outdated
* Updating all-in-one path ([@bigfleet](https://github.com/bigfleet) in [#4234](https://github.com/jaegertracing/jaeger/pull/4234))
* Migrate the use of fsnotify to fswatcher in cert_watcher.go ([@haanhvu](https://github.com/haanhvu) in [#4232](https://github.com/jaegertracing/jaeger/pull/4232))
* Restore baggage support in HotROD 🚗 ([@yurishkuro](https://github.com/yurishkuro) in [#4225](https://github.com/jaegertracing/jaeger/pull/4225))
* Jaeger YugabyteDB(YCQL) support ([@HarshDaryani896](https://github.com/HarshDaryani896) in [#4220](https://github.com/jaegertracing/jaeger/pull/4220))
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like a new feature to me, what does everyone think?

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

@albertteoh thanks for the review. PR updated

albertteoh
albertteoh previously approved these changes Mar 15, 2023
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated
@@ -13,6 +13,31 @@ next release

### UI Changes

1.43.0 (2023-03-13)
Copy link
Contributor

Choose a reason for hiding this comment

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

Update to today's date?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Pavol Loffay <[email protected]>
albertteoh
albertteoh previously approved these changes Mar 15, 2023
@pavolloffay
Copy link
Member Author

The UI test Check_if_the_favicon_icon_is_available is failing and the UI does not load if I run it locally go run -tags ui ./cmd/all-in-one/main.go

@yurishkuro
Copy link
Member

@mszabo-wikia I remember icon file moved in the UI recently

@pavolloffay
Copy link
Member Author

I think this is the root cause jaegertracing/jaeger-ui#1275

@yurishkuro
Copy link
Member

So the icon file is now legitimately gone, because it's embedded directly in HTML:

<link rel="shortcut icon" href="data:image/x-icon;base64,AAAB......A==">

However, the UI loads with a blank screen for me (tried both with and without ui prefix).

@yurishkuro
Copy link
Member

@pavolloffay do you want to just downgrade UI to 1.27.4 and get the release out? It might take time to figure out how the UI build changed.

@yurishkuro
Copy link
Member

Check_if_the_favicon_icon_is_available

I'm glad we had this test :-). It won't work going forward, but the situation with the blank screen in the UI makes me think we should alter it to look for some other specific element in the UI home page. Ideally it would be something like the Search button, which would only be generated if the correct React app is loaded, but that won't happen via curl.

@yurishkuro
Copy link
Member

I added a bit of logging to static handler, things look ok from the server side

serving /static/index-b9997246.js
serving /static/index-3c2d679b.css

and in Chrome too:
image

I think it's really something about the UI build output, the app is incomplete or we're using a wrong mode.

@mszabo-wikia any thoughts?

@mszabo-wikia
Copy link

@yurishkuro Interesting, I remember checking the production build case with a local all-in-one install & there were no issues then. Will retest that to see what changed.

@mszabo-wikia
Copy link

Looks like there's this error:

Uncaught TypeError: Unknown theme type: undefined, name: undefined
    withSuffix http://localhost:16686/static/index-b9997246.js:69

@mszabo-wikia
Copy link

Although ant-design/ant-design#19002 (comment) is in Chinese, it seems the observed error matches & from some quick local testing the UI loads properly after applying that update to the build config. So this looks to be an antd 3.9.0 -> 3.26 update regression.

@mszabo-wikia
Copy link

Filed jaegertracing/jaeger-ui#1276. ee7ce64f6784e5e00718a58680a43098f83a3c94 is the first commit that breaks without the additional config, so antd 3.26 indeed seems to be at fault.

Signed-off-by: Pavol Loffay <[email protected]>
yurishkuro
yurishkuro previously approved these changes Mar 15, 2023
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I expect favicon test would still fail, need to remove it

Signed-off-by: Pavol Loffay <[email protected]>
t.Log("Checking favicon...")
resp, err := http.Get(queryURL + "/favicon.ico")
resp, err := http.Get(queryURL + "/static/jaeger-logo-ab11f618.svg")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the hash in the filename may change over time and break this test. If it's based on the file checksum then it should be stable. We can improve the test later, e.g. request home page first, and look for logo URL in HTML.

Copy link
Member Author

Choose a reason for hiding this comment

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

the hash should not change if the logo does not change

@pavolloffay pavolloffay merged commit d1cf716 into jaegertracing:main Mar 15, 2023
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