feat: add cluster-wide message #9261

maxrussell · 2024-04-27T00:54:04Z

Ticket

Description

Admin users should be able to set a cluster-wide message that is displayed to all users for things like server maintenance.

Test Plan

This should be tested manually in Release Party; here are the acceptance criteria:

Non-admins should not be able to set the cluster message or use the det master cluster-message get CLI command (since the latter can show not-yet-active cluster messages).
Admins should be able to set the cluster message, and it should show up in the Web UI for all users.
Start and end time should work
- Messages should be able to be scheduled in the future and only show up when the start time has been reached
- Messages should end after the end time and no longer show up in det master info
--duration should work
Cluster message banner should disappear within a minute or two after the message expires.
If the browser is too narrow to fit the full text of the cluster message, its text should be truncated with an ellipsis (...) rather than make the banner taller to add lines.
The cluster message banner being present should not cause layout problems
- It is a known issue that resizing the browser intermittently causes extra scrollbars to appear. Reloading the page should cause these to go away.

Checklist

Acceptance testing completed with PM (@hkumar92)
Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.

netlify · 2024-04-27T00:54:23Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`4ffeadf`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6668e192d5cc4d0008915abb
😎 Deploy Preview	https://deploy-preview-9261--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-05-15T22:55:30Z

Codecov Report

Attention: Patch coverage is 37.92049% with 203 lines in your changes missing coverage. Please review.

Project coverage is 48.97%. Comparing base (439734b) to head (4ffeadf).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9261      +/-   ##
==========================================
- Coverage   49.00%   48.97%   -0.03%     
==========================================
  Files        1235     1238       +3     
  Lines      160191   160507     +316     
  Branches     2781     2784       +3     
==========================================
+ Hits        78494    78604     +110     
- Misses      81522    81728     +206     
  Partials      175      175

Flag	Coverage Δ
backend	`43.81% <41.55%> (-0.04%)`	⬇️
harness	`63.85% <25.00%> (-0.11%)`	⬇️
web	`44.13% <52.45%> (+0.01%)`	⬆️

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

Files	Coverage Δ
webui/react/src/components/ClusterMessage.tsx	`100.00% <100.00%> (ø)`
webui/react/src/stores/determinedInfo.tsx	`86.02% <100.00%> (+1.49%)`	⬆️
master/internal/db/postgres_clustermessage.go	`94.11% <94.11%> (ø)`
webui/react/src/App.tsx	`0.00% <0.00%> (ø)`
master/pkg/model/maintenance_message.go	`0.00% <0.00%> (ø)`
webui/react/src/components/Navigation.tsx	`0.00% <0.00%> (ø)`
webui/react/src/services/decoder.ts	`20.13% <6.66%> (-0.32%)`	⬇️
harness/determined/cli/master.py	`23.52% <19.04%> (-2.01%)`	⬇️
harness/determined/common/api/bindings.py	`40.23% <26.37%> (-0.12%)`	⬇️
master/internal/api_master.go	`2.38% <0.00%> (-1.30%)`	⬇️

... and 5 files with indirect coverage changes

docs/release-notes/cluster-message.rst

docs/tools/webui-if.rst

tara-det-ai

requested edits

webui/react/src/components/ClusterMessage.test.tsx

webui/react/src/components/Navigation.module.scss

webui/react/src/services/decoder.ts

webui/react/src/components/ClusterMessage.tsx

Co-authored-by: Tara Charter <[email protected]>

harness/determined/cli/master.py

azhou-determined

minor comments, otherwise python-side lgtm

keita-determined

added few comments but pretty good shape

also, i think this needs to be tested in release party even though there are test cases especially for webui because the test cases don't handle the entire webui layout, and its good to have human eyes on it (webui is buggy in general...)

keita-determined · 2024-06-11T18:08:34Z

webui/react/src/App.module.scss

@@ -2,6 +2,6 @@
  background-color: var(--theme-background);
  display: flex;
  flex-direction: column;
-  height: 100%;
+  max-height: 90vh;


qq: do we need this change?

webui/react/src/components/ClusterMessage.test.tsx

webui/react/src/components/ClusterMessage.tsx

keita-determined · 2024-06-11T19:10:08Z

webui/react/src/components/ClusterMessage.tsx

+  message?: ClusterMessage;
+}
+
+const ClusterMessageBanner: React.FC<Props> = ({ message }) => {


refactor

diff --git a/webui/react/src/components/ClusterMessage.module.scss b/webui/react/src/components/ClusterMessage.module.scss index 5eb3396b72..d9b0284d9f 100644 --- a/webui/react/src/components/ClusterMessage.module.scss +++ b/webui/react/src/components/ClusterMessage.module.scss @@ -5,20 +5,21 @@ $height: 40px; background-color: #b44; color: #ddd; display: inline-flex; + gap: 4px; // proper value 2px? 4px? 1ch? height: $height; justify-content: center; min-height: $height; padding: 0 1em; width: 100%; -} -.base > * { - max-height: 100%; // probably not needed - overflow: hidden; - text-align: center; - text-overflow: ellipsis; - white-space: nowrap; + + > span { + max-height: 100%; + overflow: hidden; + text-align: center; + text-overflow: ellipsis; + white-space: nowrap; + } } .clusterMessageLabel { font-weight: bold; - width: 60%; } diff --git a/webui/react/src/components/ClusterMessage.tsx b/webui/react/src/components/ClusterMessage.tsx index 9825445058..fa5e9c5a3d 100644 --- a/webui/react/src/components/ClusterMessage.tsx +++ b/webui/react/src/components/ClusterMessage.tsx @@ -11,12 +11,10 @@ interface Props { const ClusterMessageBanner: React.FC<Props> = ({ message }) => { return message ? ( <div className={css.base}> - <span> - <span className={css.clusterMessageLabel} data-testid="admin-msg"> - Message from Admin: - </span>{' '} - <span data-testid="cluster-msg">{message.message}</span> + <span className={css.clusterMessageLabel} data-testid="admin-msg"> + Message from Admin: </span> + <span data-testid="cluster-msg">{message.message}</span> </div> ) : null; };

keita-determined · 2024-06-11T19:11:30Z

webui/react/src/components/ClusterMessage.tsx

+        <span className={css.clusterMessageLabel} data-testid="admin-msg">
+          Message from Admin:
+        </span>{' '}
+        <span data-testid="cluster-msg">{message.message}</span>


qq: do we wanna show the default string when message.message is empty string?

Ah, good catch. I think we should only show the banner when the message has length > 0.

webui/react/src/components/Navigation.module.scss

keita-determined · 2024-06-11T19:27:37Z

webui/react/src/components/Navigation.tsx

@@ -16,11 +16,11 @@ import css from './Navigation.module.scss';
 import NavigationSideBar from './NavigationSideBar';
 import NavigationTabbar from './NavigationTabbar';

-interface Props {
-  children?: React.ReactNode;
+interface Props extends PropsWithChildren {


qq: why do we wanna use PropsWithChildren? i think children is preferable

That was chosen so we could also pass clusterMessagePresent, which isn't a child. Is there a better way to do that?

interface Props { children?: React.ReactNode; isClusterMessagePresent?: boolean; }

i think this should work

keita-determined · 2024-06-11T19:28:06Z

webui/react/src/components/Navigation.tsx

-interface Props {
-  children?: React.ReactNode;
+interface Props extends PropsWithChildren {
+  clusterMessagePresent?: boolean;


Suggested change

clusterMessagePresent?: boolean;

isClusterMessagePresent?: boolean;

keita-determined · 2024-06-11T19:31:18Z

webui/react/src/services/decoder.ts

@@ -90,10 +90,24 @@ export const mapV1MasterInfo = (data: Sdk.V1GetMasterResponse): DeterminedInfo =
    return acc;
  }, BrandingType.Determined);

+  const clustMsg = data.clusterMessage


Suggested change

const clustMsg = data.clusterMessage

const clustMsg: ClusterMessage | undefined = data.clusterMessage

keita-determined · 2024-06-11T19:33:54Z

webui/react/src/components/Page.module.scss

@@ -31,7 +31,7 @@
    }
  }
  &.noScroll {
-    max-height: 100vh;
+    max-height: 80vh;


qq: do we need to change this?

eecsliu · 2024-06-17T20:13:17Z

webui/react/src/components/ClusterMessage.test.tsx

+    expect(screen.getByTestId('cluster-msg')).toBeInTheDocument();
+
+    // make sure the cluster message is visible, but the admin msg is not.
+    expect(screen.getByText(msg)).toBeInTheDocument();


We care about the content as opposed to the element. In this test case we care about the text content as well, os we should add a check for the actual content.
@amandavialva01 @keita-determined

use testID then get by text inside the test ID element

eecsliu · 2024-06-17T20:21:54Z

master/internal/db/postgres_clustermessage.go

+// only one cluster message is allowed at any time. Messages may be at most ClusterMessageMaxLength
+// characters long. Returns a wrapped ErrInvalidInput when input is invalid.
+func SetClusterMessage(ctx context.Context, db *bun.DB, msg model.ClusterMessage) error {
+	if msgLen := utf8.RuneCountInString(msg.Message); msgLen > ClusterMessageMaxLength {


maybe we could put a 0 check here? @amandavialva01

cla-bot bot added the cla-signed label Apr 27, 2024

maxrussell force-pushed the max/maint-msg-fresh branch 2 times, most recently from 01cd526 to 3658f57 Compare May 7, 2024 23:23

tara-det-ai self-requested a review May 15, 2024 20:25

maxrussell force-pushed the max/maint-msg-fresh branch from ef5efe8 to 3a4ff9d Compare May 15, 2024 22:58

determined-ci requested a review from a team May 16, 2024 21:54

determined-ci added the documentation Improvements or additions to documentation label May 16, 2024

maxrussell force-pushed the max/maint-msg-fresh branch from ec89801 to 4e2d53f Compare May 20, 2024 18:55

determined-ci removed the documentation Improvements or additions to documentation label May 20, 2024

tara-det-ai mentioned this pull request May 20, 2024

docs: Describe admin mtc banner #9373

Closed

4 tasks

maxrussell force-pushed the max/maint-msg-fresh branch from 91fb01a to f5afa9f Compare June 4, 2024 01:45

determined-ci added the documentation Improvements or additions to documentation label Jun 4, 2024

maxrussell force-pushed the max/maint-msg-fresh branch from 712d3e9 to 8b5e2d5 Compare June 6, 2024 17:54

tara-det-ai reviewed Jun 6, 2024

View reviewed changes

docs/release-notes/cluster-message.rst Outdated Show resolved Hide resolved

tara-det-ai reviewed Jun 6, 2024

View reviewed changes

docs/tools/webui-if.rst Show resolved Hide resolved

tara-det-ai approved these changes Jun 6, 2024

View reviewed changes

determined-ci requested a review from a team June 6, 2024 18:52

maxrussell added the User-facing API Change label Jun 8, 2024

maxrussell changed the title ~~draft: feat: add cluster-wide message~~ feat: add cluster-wide message Jun 8, 2024

maxrussell requested review from eecsliu, a team, keita-determined and azhou-determined June 8, 2024 00:58

maxrussell marked this pull request as ready for review June 8, 2024 00:59

maxrussell requested review from a team as code owners June 8, 2024 00:59

keita-determined reviewed Jun 10, 2024

View reviewed changes

tara-det-ai and others added 11 commits June 10, 2024 21:48

Content edits

ac7b777

Docs and release notes

4ac9da6

bump migration date

f055005

Docs updates

d2e3ce8

Co-authored-by: Tara Charter <[email protected]>

new test cases and some cleanup

a5fadf8

More cleanup

0f7f93a

linting

fbda27c

even more lint

2c26308

cleanup: stop using TypeScript as and removed unnecessary fragment

4210ecb

more CSS cleanup

69f8c9a

ClusterMessage component test fixup

1a2341d

maxrussell force-pushed the max/maint-msg-fresh branch from 1ca4002 to 1a2341d Compare June 11, 2024 04:49

maxrussell added 5 commits June 10, 2024 21:51

test polish

e1d944f

Some lint I guess 😔

69365f3

even more linting

c2bd509

Docs lint

9c7e407

web lint

a5863aa

azhou-determined reviewed Jun 11, 2024

View reviewed changes

harness/determined/cli/master.py Show resolved Hide resolved

maxrussell requested a review from keita-determined June 11, 2024 19:16

azhou-determined reviewed Jun 11, 2024

View reviewed changes

harness/determined/cli/master.py Outdated Show resolved Hide resolved

azhou-determined approved these changes Jun 11, 2024

View reviewed changes

keita-determined reviewed Jun 11, 2024

View reviewed changes

keita-determined approved these changes Jun 11, 2024

View reviewed changes

maxrussell added 2 commits June 11, 2024 12:43

CLI cleanup

fcf63b2

for the love of lint

4ffeadf

maxrussell enabled auto-merge (squash) June 11, 2024 23:58

maxrussell merged commit 86e6b68 into main Jun 12, 2024
81 of 99 checks passed

maxrussell deleted the max/maint-msg-fresh branch June 12, 2024 00:12

eecsliu reviewed Jun 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add cluster-wide message #9261

feat: add cluster-wide message #9261

maxrussell commented Apr 27, 2024 •

edited

Loading

netlify bot commented Apr 27, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

tara-det-ai left a comment

azhou-determined left a comment

keita-determined left a comment

keita-determined Jun 11, 2024

keita-determined Jun 11, 2024 •

edited

Loading

keita-determined Jun 11, 2024 •

edited

Loading

maxrussell Jun 11, 2024

keita-determined Jun 11, 2024

maxrussell Jun 11, 2024

keita-determined Jun 11, 2024

keita-determined Jun 11, 2024

keita-determined Jun 11, 2024

keita-determined Jun 11, 2024

eecsliu Jun 17, 2024

eecsliu Jun 17, 2024

eecsliu Jun 17, 2024

	clusterMessagePresent?: boolean;
	isClusterMessagePresent?: boolean;

	const clustMsg = data.clusterMessage
	const clustMsg: ClusterMessage \| undefined = data.clusterMessage

feat: add cluster-wide message #9261

feat: add cluster-wide message #9261

Conversation

maxrussell commented Apr 27, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 27, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented May 15, 2024 • edited Loading

Codecov Report

tara-det-ai left a comment

Choose a reason for hiding this comment

azhou-determined left a comment

Choose a reason for hiding this comment

keita-determined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keita-determined Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

keita-determined Jun 11, 2024 • 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

maxrussell commented Apr 27, 2024 •

edited

Loading

netlify bot commented Apr 27, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

keita-determined Jun 11, 2024 •

edited

Loading

keita-determined Jun 11, 2024 •

edited

Loading