-
-
Notifications
You must be signed in to change notification settings - Fork 165
BE: Allow disabling GitHub release info #1108
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,43 @@ | ||
package io.kafbat.ui.service; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
||
import io.kafbat.ui.AbstractIntegrationTest; | ||
import io.kafbat.ui.util.DynamicConfigOperations; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
|
||
class ApplicationInfoServiceTest extends AbstractIntegrationTest { | ||
@Autowired | ||
private ApplicationInfoService service; | ||
|
||
@Autowired | ||
private DynamicConfigOperations dynamicConfigOperations; | ||
|
||
@Test | ||
void testCustomGithubReleaseInfoTimeout() { | ||
assertEquals(100, service.githubReleaseInfo().getGithubApiMaxWaitTime()); | ||
} | ||
|
||
@Test | ||
void testDisabledReleaseInfo() { | ||
stklcode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var service2 = new ApplicationInfoService( | ||
dynamicConfigOperations, | ||
null, | ||
null, | ||
null, | ||
false, | ||
101 | ||
); | ||
|
||
assertNull(service2.githubReleaseInfo(), "unexpected GitHub release info when disabled"); | ||
var appInfo = service2.getApplicationInfo(); | ||
assertNotNull(appInfo, "application info must not be NULL"); | ||
assertNull(appInfo.getLatestRelease(), "latest release should be NULL when disabled"); | ||
assertNotNull(appInfo.getBuild(), "build info must not be NULL"); | ||
assertNotNull(appInfo.getEnabledFeatures(), "enabled features must not be NULL"); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ const Version: React.FC = () => { | |
|
||
return ( | ||
<S.Wrapper> | ||
{!isLatestRelease && ( | ||
{isLatestRelease === false && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see a visual difference between "no latest release present" and "user disabled version check" in the UI. This is crucial as it's often only the UI screenshots I have to deduct things from. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any suggestion how such information should look? Frankly, I don't see any benefit in such information. Currently running version is displayed, if the check failed there's a warning symbol and the information if there's an update available or not can change 1 second after a screenshot was taken, because it's dynamic info. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a red (rather than yellow) exclamation mark icon (most likely it's an svg, so altering stroke-color should work) with a different text about version check disabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A red warning, really? I've learned to ignore the yellow icon due do failing version checks and yet there are frequent questions if there's something wrong. So annoying users, who have no control whatsoever about the deployed version in our case, with read warning sign that typically means "something is wrong here" ... Our use case are - you guessed it - isolated systems, mostly dev/staging environments. We allow users some access to the underlying Kafka and deployment/updates are done by some automation/opt team who don't see the UI warnings anyway. We were looking for a solution to remove the frontend warning, not making it look worse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You wouldn't imagine the number of issues I've seen where people come with their "up-to-date" installations, only to realize after wasting some time that it's not only an outdated version, but also a fork with completely unrelated functionality that we don't even have. Not sure of a middle ground here. @germanosin thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extended the Override to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could hide the whole icon tbh. Even if the person reporting issues is not the same as the one managing the installation, they can still check if the version is up to date by clicking the commit hash. Given the counterpoint by @stklcode about "users screaming due to an alert", it might be alright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any update on this one? I'd be happy to remove the latest commit again (hide the icon) if we agree on that or keep is as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stklcode ok let's hide the icon (based on a property ofc) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I removed the latest commit, so now the icon is gone if I will extract a minor change (warning icon color from theme) into a separate PR, as it is now little out of scope here. (#1282) |
||
<S.OutdatedWarning | ||
title={`Your app version is outdated. Latest version is ${ | ||
versionTag || 'UNKNOWN' | ||
|
Uh oh!
There was an error while loading. Please reload this page.