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

feat(core): add application-specific custom banners #6450

Merged
merged 1 commit into from Feb 5, 2019

Conversation

Projects
None yet
7 participants
@maggieneterval
Copy link
Contributor

maggieneterval commented Jan 30, 2019

Let me know if you all have any thoughts on making the custom banners more or less configurable -- the specific use case this was requested for was putting up production freeze or maintenance alerts on specific applications.

b5455002-3461-4792-b8f9-c8e06eae1b1f

@maggieneterval maggieneterval force-pushed the maggieneterval:custom-banner branch from 9f4125d to 19ccca0 Jan 30, 2019

@maggieneterval maggieneterval requested review from sbwsg , gcomstock and jtk54 Jan 30, 2019

@bpowell

This comment has been minimized.

Copy link

bpowell commented Jan 30, 2019

Awesome feature! We had something like this on the application I used to work on, it was extremely helpful.

Is there anyway to add a link to documentation or something so people know why the freeze/maintenance is happening?

@jtk54

jtk54 approved these changes Jan 30, 2019

Copy link
Contributor

jtk54 left a comment

LGTM. I second the link idea, but I think that warrants more thought. Is there only one link configured, or are they configured for some combination of app/cloud provider?

@bpowell

This comment has been minimized.

Copy link

bpowell commented Jan 30, 2019

After talking to @ethanfrogers about this, a cool idea we came up with would be to have an emergency alert system. For example, PageDuty is alerting about a system being down, have that reflect in the Spinnaker UI for that application. I'd say this is more of a v2 or v3 feature for this, but figured I should bring it up

@anotherchrisberry

This comment has been minimized.

Copy link
Contributor

anotherchrisberry commented Jan 30, 2019

This looks really cool.

Could you also look at the application's attributes from Front50 to see if a banner is configured there? Not sure what the UI would look like, maybe another section on the config tab. But this would give application owners some control over the banners, and avoid having to redeploy Deck to make changes to banners.

@sbwsg

sbwsg approved these changes Jan 30, 2019

@gcomstock

This comment has been minimized.

Copy link
Contributor

gcomstock commented Jan 30, 2019

Looks nice! Just make sure the extra height/offset is compatible with the blesk messaging, and doesn't push the scrollable sections down and cause cropping.

@maggieneterval

This comment has been minimized.

Copy link
Contributor Author

maggieneterval commented Jan 30, 2019

Thanks all for the feedback! Adding links and automatic alert functionality sound like a good v2 for this -- hopefully we get some user feedback to inform what might be most helpful.

@anotherchrisberry I like the idea of moving the banner configuration out of settings and into an application's Front50 attributes -- I'll put some thought into the UI for this on the config tab and post another screenshot once I've updated this PR.

@maggieneterval

This comment has been minimized.

Copy link
Contributor Author

maggieneterval commented Jan 30, 2019

@gcomstock naive question for you -- what is the blesk messaging?

@gcomstock

This comment has been minimized.

Copy link
Contributor

gcomstock commented Jan 30, 2019

@gcomstock naive question for you -- what is the blesk messaging?

Ah Sorry Maggie, I've been informed its a Netflix-specific banner that also goes across the top and pushes the page down.

@maggieneterval maggieneterval force-pushed the maggieneterval:custom-banner branch from 19ccca0 to e38a237 Feb 4, 2019

@maggieneterval

This comment has been minimized.

Copy link
Contributor Author

maggieneterval commented Feb 4, 2019

Just pushed an update to support configuring application banners from the Config tab -- let me know what you think of the UI. After some discussion here we decided it would be best to limit users to several Spinnaker color palette options for the banner text and background color.

@anotherchrisberry or @sbwsg this is an angular newb question -- is there a preferred method in Deck for communicating among components without a close ancestor, so that we can trigger an update in the header after a new banner config is enabled? Currently the user would need to refresh the page for an updated banner to take effect. Thanks!

@maggieneterval maggieneterval force-pushed the maggieneterval:custom-banner branch from e38a237 to ceca704 Feb 4, 2019

@jtk54

This comment has been minimized.

Copy link
Contributor

jtk54 commented Feb 4, 2019

@maggieneterval is this something we expect to be able to control via spin app save?

@maggieneterval

This comment has been minimized.

Copy link
Contributor Author

maggieneterval commented Feb 4, 2019

@jtk54 can definitely add a spin command for this if you think it would be useful!

@jtk54

This comment has been minimized.

Copy link
Contributor

jtk54 commented Feb 4, 2019

Actually judging from what's there so far users will be able to submit app banner config as long as it's discoverable somehow. I think at the moment folks can inspect the traffic to see the payload sent but it might be worth documenting or adding a 'See application json' similarly to pipelines.

@maggieneterval maggieneterval force-pushed the maggieneterval:custom-banner branch from ceca704 to fdf2010 Feb 4, 2019

@anotherchrisberry

This comment has been minimized.

Copy link
Contributor

anotherchrisberry commented Feb 5, 2019

I think this looks great! I'm not sure if you could use the formik forms stuff that @christopherthielen and @alanmquach have been working on, but that might also be a nice TODO later.

@maggieneterval maggieneterval force-pushed the maggieneterval:custom-banner branch from fdf2010 to e38b65c Feb 5, 2019

@maggieneterval maggieneterval merged commit c6a3528 into spinnaker:master Feb 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maggieneterval maggieneterval deleted the maggieneterval:custom-banner branch Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment