Implement announcements as data-driven content #25769

sftim · 2020-12-22T22:28:02Z

Rather than configuring announcements in the config.toml, add a mechanism to handle these as data driven content.
This change allows announcements to have an expiry date and / or a “do not show until” date. You can also have multiple announcements, for example to have a Kubecon announcement that takes priority but only until the end of the conference.

Part of the implementation for issue #25759. Preview (but it doesn't show any current announcement).

/kind feature
/area web-development

/hold
because to make the expiry date mechanism work, we should also set up triggered builds. That'll be a separate pull request.

netlify · 2020-12-22T22:37:53Z

✔️ Deploy preview for kubernetes-io-master-staging ready!

🔨 Explore the source changes: 0b6280f

🔍 Inspect the deploy logs: https://app.netlify.com/sites/kubernetes-io-master-staging/deploys/5ff712162a9b520007bf08da

😎 Browse the preview: https://deploy-preview-25769--kubernetes-io-master-staging.netlify.app

sftim · 2020-12-22T22:44:41Z

@kbhawkey & @onlydole - if either of you have time to review this, I'd welcome your thoughts.

sftim · 2020-12-22T22:45:22Z

BTW ~~we can't add an OWNERS file inside /data/announcements because Hugo wants to parse all those files as, er, data.~~

onlydole

A big +1 for time bound announcements! This is a great implementation and improvement, @sftim! Thank you for working on this 🌟

k8s-ci-robot · 2020-12-22T22:51:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: onlydole

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [onlydole]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sftim · 2020-12-22T22:51:15Z

layouts/partials/frontpage-announcement.html

-{{ if .Page.Param "announcement" }}
-<section lang="en" id="fp-announcement" style="background-color:{{ .Page.Param "announcement_bg" }}">
-  <aside >
+{{ $announcementShown := false }}


I considered refactoring this to be a single layout or partial that sets a different ID for the front page, or even making the HTML the same for both cases and using CSS to adjust styling. I decided that'd be best saved for a different PR; this one already has an potential impact on every page on the site(!) so let's keep it narrow.

sftim · 2020-12-22T22:52:21Z

(forced a Netlify retry after a Hugo concurrency error)

sftim · 2020-12-22T23:44:48Z

The other part of this change is PR #25771: set up GitHub Actions to trigger a deploy for this website, twice per day.

kbhawkey · 2020-12-23T00:30:16Z

👀

kbhawkey · 2020-12-23T01:16:59Z

layouts/partials/announcement.html

+{{ $announcementShown := false }}
+
+{{ $announcements := $.Site.Data.announcements }}
+{{ range sort $announcements "spec.priority" "desc" }}


@sftim, Some initial feedback/questions about the data and how to use this content:

You envision a number of unique, reusable announcement files stored as data. Each file must be parsed in these partial templates in order to find the single announcement to publish? Any contributor can create a data file and have their announcement published based on the dates set in the file?

Do you need to set a priority? Seems complicated and possibly error prone. Could you use a boolean (publish = true)? How many announcements can you publish at one time?

It might be nice to have a base data file for all announcements and then have other announcements
override fields as needed.

I see an overlap of code in the two partial templates. Could you write another template that is used by both templates?

If these are defaults, style="background-color: #3371e3; color: #fff;, could add to a base (announcement) data file?

How will the announcement be removed? Could the announcement have start and expiration dates? Is that described in the fields: notAfter and notBefore? When an announcement expires, the "default" or base announcement (could be nothing) is displayed.

Can the announcement message be localized?

Will a periodic job read data files (or some other job configuration files) to determine what/when to publish (blog page)?

You envision a number of unique, reusable announcement files stored as data. Each file must be parsed in these partial templates in order to find the single announcement to publish? Any contributor can create a data file and have their announcement published based on the dates set in the file?

I could add a note to the website contributor guide about how to set and review announcements. Maybe that'd work.
Only the steering committee can approve these announcements (see issue #25759 for details). This PR doesn't implement any enforcement mechanism for ~~details~~ messages.

Do you need to set a priority? Seems complicated and possibly error prone.

We have had announcements override other announcements (eg: KubeCon, overriding the current banner on the topic of Black Lives Matter). To me it seems like the override case is easier than the alternative: splitting the outer announcement into two and having three defined announcements that just avoid overlapping.

It might be nice to have a base data file for all announcements and then have other announcements
override fields as needed.

There's one required field, .spec.message. I don't think there's a sensible default to set for that.
As with the current implementation, the default is not to show an announcement.

I see an overlap of code in the two partial templates. Could you write another template that is used by both templates?

I explained my thinking in #25769 (comment)

If these are defaults, style="background-color: #3371e3; color: #fff;, could add to a base (announcement) data file?

That sounds like more complexity for a really niche case. I envisage an implementation of that covering an override mechanism for every other field to ensure a override mechanism for one. I don't think it's justified, especially as CSS is the easiest field to define overrides for (it's kind of built-in).

How will the announcement be removed?

Removal is optional; it'll improve site build time but I suspect not by much.

Could the announcement have start and expiration dates? Is that described in the fields: notAfter and notBefore?

Yes. Those fields are optional.

When an announcement expires, the "default" or base announcement (could be nothing) is displayed.

As with the current implementation, the default is not to show an announcement.

Can the announcement message be localized?

I haven't implemented this. Instead, as with the current implementation, we mark the HTML elements for the announcement as English (overriding any page-level setting, and helping any web crawlers that care).

Will a periodic job read data files (or some other job configuration files) to determine what/when to publish (blog page)?

For blog articles, Hugo has built-in support for not publishing articles before their publication date. PR #25771 implements an automatic build trigger.
Those two mechanisms combined would make blog articles go live when ready (we use a similar mechanism for https://www.scalefactory.com/blog/).
This PR together with #25771 would make announcements go live or disappear as needed. See that PR for details about the approach.

reverse-sorting by start date seems like it would be a simpler than a separate priority field, and I think it would do what you want in the case of a long-lived announcement being temporarily overridden by a short-lived announcement

Can the announcement message be localized?

I also had this question, though the current localization had very limited utilization (only korean localized it, looks like), and the short-lived nature of announcements seem unlikely to mesh well with slower translation cycles.

reverse-sorting by start date

There could be some gotchas around sorting on start date; Hugo date parsing and handling is there, but it is a bit raw. New Hugo releases are adding useful time and date features so the topic is definitely getting attention.

To be honest I already implemented priority and I known that'll work. The start date thing seems like it could be a surprise to people who want to work out which announcement will show when a current one elapses, whereas a priority value is easy to teach.

ok, I don't feel strongly. I don't really expect there to be more than 1-2 at any given time

I guess the site will not substitute an "empty" section when there are no announcements?

@kbhawkey examples of the priority field:

--- priority: 10 dateConstraints: startTime: 2020-01-01T00:00:00 endTime: 2022-07-01T00:00:00 message: |- Kubernetes all through 2020 _and_ the first half of 2021.

and

--- priority: 20 dateConstraints: startTime: 2021-03-20T09:00:00 endTime: 2021-04-02T00:00:00 style: >- color: #fff; background: #505; title: KubeCon Offworld 2021 message: |- Kubecon Offworld 2021 will be held on the planet Marklar from the first of April to the first of April.

The Kubecon banner would override the “Kubernetes all through 2020 and the first half of 2021.” banner for the configured duration.

I guess the site will not substitute an "empty" section when there are no announcements?

If there is no active announcement then there's no section#announcement emitted; that's the same as what the current mechanism does.

Yes, I understand that you want to sort by priority instead of looking for a "publish" field.

sftim · 2020-12-23T16:34:55Z

BTW, merging this PR as it stands removes the existing banner. I can / will add another commit that retains it, when that's useful to do.

liggitt · 2020-12-23T17:08:48Z

data/announcements/example.yaml

+apiVersion: docs.k8s.io/v1alpha1
+kind: Announcement
+spec:


does this do anything currently? if not, suggest removing it... it'll just get copy/pasted and not add much value

The example.yaml? It's an example.

If you mean .spec then you can't omit it with the current code. IMO it makes the Hugo layout code a bit nicer because I can {{ with .spec }} to bring just those fields into scope.

If you mean the apiVersion and kind then the current code enforces that you are using this particular format, but we can remove that validation if it's not useful.

the apiVersion/kind/spec bits... I didn't see any enforcement there, so it didn't seem like it was accomplishing anything

a flat structure seemed easier to read, but maybe that's just me:

# priority is required # Largest number wins tie break if there are multiple announcements priority: 0 # start/stop are required, and must be in format YYYY-MM-DDTHH:MM:SS startDateTime: "2020-01-01T00:00:00" stopDateTime: "2020-04-01T00:00:00" # message is required message: |- Marklar marklar marklar marklar marklar. Marklar marklar. [Marklar](https://en.wikipedia.org/wiki/Starvin%27_Marvin_in_Space#Plot)! # title is optional title: Sample announcement # style is optional; if using, set both color and background style: "color: #fff; background: #000;"

does this do anything currently? if not, suggest removing it... it'll just get copy/pasted and not add much value

+1 for removing any unused fields

OK, I can try to simplify it.

Ah, I did have validation in before but must have left that out.

It's too bad that the title and message are not localized. I assume you can use a template in the data file.
I have not thought this through ...

[announcement] [announcement.<name>] title = "Kubernetes Europe" message = "Come to Kubecon"

data/announcements/example.yaml

liggitt · 2020-12-23T18:26:25Z

this still is looking at the old config value:

{{- if .Site.Params.announcement }}
<link rel="stylesheet" href="{{ "css/announcement.css" | relURL }}">
{{- end }}

sftim · 2020-12-23T19:01:16Z

{{- if .Site.Params.announcement }}
<link rel="stylesheet" href="{{ "css/announcement.css" | relURL }}">
{{- end }}

Ah, good spot.

kbhawkey · 2021-01-05T15:01:48Z

data/announcements/scheduled.yaml

+# 🛈  Changes require approval from @kubernetes/steering-committee
+
+# This is just an example. For the actual announcements, see
+# announcements.yaml


Is there a reason to have this file (template and docs) and a separate file for the announcement data?
What do you think about combining the instructions and template with the data?

What do you think about combining the instructions and template with the data?

Agree, especially if the data has to be a in a single list in a single file.

It is possible to have multiple files, though, I find it easier if the data is in one place or presented as a single view.

sftim · 2021-01-07T08:39:07Z

I probably won't have much time to spend on improvements to this PR. If it's pretty much good enough to merge, let's ship it and be prepared to follow up with any important tweaks.

This change allows announcements to have an expiry date and / or a “do not show until” date. Separating this out also leaves room for future changes to enforce a set of approvers. Co-Authored-By: Karen Bradshaw <[email protected]>

sftim · 2021-01-07T09:06:47Z

/hold cancel

the automatic builds from PR #25771 are now working

This PR looks ready to supersede the implementation from PR #16210.

ℹ️ I have updated to include the existing banner text. As the new mechanism enforces a future expiry date, I picked one. Once this merges, further changes to announcements need authorization from by @kubernetes/steering-committee.

kbhawkey · 2021-01-07T13:15:00Z

data/announcements/scheduled.yaml

+
+# leave the "announcements" key in place
+announcements:
+  - startTime: 2020-06-04T23:04:39


@sftim,
Is there a good reason to keep the existing announcement or should this PR only include the example template?
I don't see a strong case to place an announcement with this PR.
An announcement can be added in a separate PR (and approved separately by the Steering Committee).
This change is about changing the way the announcements are created and approved by setting up the announcements in a data file and processing the delivery of the announcements.

sftim · 2021-01-07T13:25:07Z

Is there a good reason to keep the existing announcement or should this PR only include the example template?

I said I'd revise this PR to retain the existing announcement.
If we merge this without migrating the current configuration, this PR removes the existing announcement. That removal is a decision for someone else.

kbhawkey · 2021-01-07T13:38:13Z

Is there a good reason to keep the existing announcement or should this PR only include the example template?

I said I'd revise this PR to retain the existing announcement.
If we merge this without migrating the current configuration, this PR removes the existing announcement. That removal is a decision for someone else.

You are referring to this,
"BTW, merging this PR as it stands removes the existing banner. I can / will add another commit that retains it, when that's useful to do."

I think that is confusing. I read through this PR (many times) and found that you only proposed sample data.
Perhaps your intention has always been to keep the existing announcement, but that you forgot to put the
data back.

liggitt · 2021-01-07T13:38:53Z

I probably won't have much time to spend on improvements to this PR. If it's pretty much good enough to merge, let's ship it and be prepared to follow up with any important tweaks.

I agree.

Is there a good reason to keep the existing announcement or should this PR only include the example template?

I said I'd revise this PR to retain the existing announcement.
If we merge this without migrating the current configuration, this PR removes the existing announcement. That removal is a decision for someone else.

Per the announcement policy, the current banner, re-enabled on 11/23/2020, should have an expiry of 12/14/2020.

sftim · 2021-01-07T13:50:29Z

I'll revise the expiry date, thanks @liggitt

sftim · 2021-01-07T13:50:57Z

Actually not much point adding an expired banner!

kbhawkey · 2021-01-07T16:54:17Z

i18n/en.toml

@@ -1,11 +1,5 @@
 # i18n strings for the English (main) site.
 # NOTE: Please keep the entries in alphabetical order when editing
-[announcement_title]
-other = "Black lives matter."
-


nit: Could add this announcement data into the announcement template.

see discussion of current announcement data in #25769 (comment)

kbhawkey · 2021-01-07T16:55:35Z

@sftim , thanks for working on this.
/lgtm
Hold for reviews
/hold

k8s-ci-robot · 2021-01-07T16:55:50Z

LGTM label has been added.

Git tree hash: 89addbd4d5f5fd0a18f1972923ea91592af4f3c2

liggitt · 2021-01-07T16:57:09Z

/lgtm

sftim · 2021-01-07T17:06:04Z

This looks reviewed to me. #25769 (comment) is an LGTMing review from a steering committee member, and https://github.com/kubernetes/website/pulls#pullrequestreview-557411184 is an approving review from a SIG Docs tech lead.

/unhold

sftim · 2021-03-28T19:13:15Z

Also see PR #27274

k8s-ci-robot requested review from celestehorgan and kbarnard10 December 22, 2020 22:28

k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Dec 22, 2020

sftim force-pushed the 20201222_data_driven_announcements branch from 9f54245 to 9e14354 Compare December 22, 2020 22:49

onlydole approved these changes Dec 22, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2020

sftim commented Dec 22, 2020

View reviewed changes

kbhawkey reviewed Dec 23, 2020

View reviewed changes

kbhawkey mentioned this pull request Dec 23, 2020

Split out announcement config, enable date-bounding #25759

Closed

liggitt reviewed Dec 23, 2020

View reviewed changes

data/announcements/example.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 23, 2020

sftim force-pushed the 20201222_data_driven_announcements branch from 538dbe0 to 728e590 Compare December 23, 2020 19:11

kbhawkey reviewed Jan 5, 2021

View reviewed changes

sftim force-pushed the 20201222_data_driven_announcements branch from da1f784 to 9c0f9fa Compare January 7, 2021 08:39

sftim and others added 2 commits January 7, 2021 08:44

Implement announcements as data-driven content

20d8b28

This change allows announcements to have an expiry date and / or a “do not show until” date. Separating this out also leaves room for future changes to enforce a set of approvers. Co-Authored-By: Karen Bradshaw <[email protected]>

Move announcement styles into Sass

c02410d

sftim force-pushed the 20201222_data_driven_announcements branch from 9c0f9fa to 2bdc11d Compare January 7, 2021 08:56

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2021

kbhawkey reviewed Jan 7, 2021

View reviewed changes

sftim added 2 commits January 7, 2021 13:51

Remove legacy announcement config

db4f1d5

Require steering committee approval for announcement changes

0b6280f

sftim force-pushed the 20201222_data_driven_announcements branch from 2bdc11d to 0b6280f Compare January 7, 2021 13:52

kbhawkey reviewed Jan 7, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2021

k8s-ci-robot assigned kbhawkey Jan 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021

k8s-ci-robot assigned liggitt Jan 7, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2021

k8s-ci-robot merged commit 20afe7f into kubernetes:master Jan 7, 2021

sftim deleted the 20201222_data_driven_announcements branch January 7, 2021 17:08

Implement announcements as data-driven content #25769

Implement announcements as data-driven content #25769

Conversation

sftim commented Dec 22, 2020 • edited Loading

netlify bot commented Dec 22, 2020 • edited Loading

sftim commented Dec 22, 2020

sftim commented Dec 22, 2020 • edited Loading

onlydole left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 22, 2020

Choose a reason for hiding this comment

sftim commented Dec 22, 2020

sftim commented Dec 22, 2020

kbhawkey commented Dec 23, 2020

kbhawkey Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

sftim Dec 23, 2020 • 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

sftim commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Dec 23, 2020 • 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

liggitt commented Dec 23, 2020

sftim commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sftim commented Jan 7, 2021

sftim commented Jan 7, 2021

kbhawkey Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

sftim commented Jan 7, 2021 • edited Loading

kbhawkey commented Jan 7, 2021

liggitt commented Jan 7, 2021 • edited Loading

sftim commented Jan 7, 2021

sftim commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbhawkey commented Jan 7, 2021

k8s-ci-robot commented Jan 7, 2021

liggitt commented Jan 7, 2021

sftim commented Jan 7, 2021

sftim commented Mar 28, 2021

sftim commented Dec 22, 2020 •

edited

Loading

netlify bot commented Dec 22, 2020 •

edited

Loading

sftim commented Dec 22, 2020 •

edited

Loading

kbhawkey Dec 23, 2020 •

edited

Loading

sftim Dec 23, 2020 •

edited

Loading

liggitt Dec 23, 2020 •

edited

Loading

kbhawkey Jan 7, 2021 •

edited

Loading

sftim commented Jan 7, 2021 •

edited

Loading

liggitt commented Jan 7, 2021 •

edited

Loading