Custom branding [packit-deploy] #12

david-mears-2 · 2025-05-01T17:53:09Z

This branch deals with all custom branding config. Twinned with mrc-ide/packit#202

How custom branding works

This should be clear from the 'Custom branding config' section of README.md; let me know if not.

Test this branch locally

Follow the steps in the README, using the provided example config basicauthcustombrand to see the app with custom branding applied, and any other (e.g. basicauth) to see that it still works without branding configuration.

When you do packit start <config> make sure to add the --pull option so that you're using the latest version of the packit branch. When using any basicauth-based config, you'll need to use the ./scripts/create-super-user script if you want to see the app in a logged-in state.

Other changes

Constellation version is upgraded so we can make use of bind mounts. Bind mounts are useful here because they let us easily mount files, rather than directories (so we don't end up mounting over the whole public folder...), and it's more complicated to write a file into a volume. (Options to do that include getting a special volume driver that allows pre-populating, or using a temporary container with the file bind-mounted and copying it into the volume there.)

As a result of upgrading the constellation version, the test workflow explicitly pulls images. This is due to a change in constellation 1.2.3 - see vimc/orderly-web-deploy#56 for an analogous fix.

Before merging

Revert references to mrc-6035-custom-branding to main after mrc-6035-custom-branding is merged in packit

codecov · 2025-05-02T13:32:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (82904c5) to head (d87bdff).
Report is 34 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   91.93%   95.12%   +3.19%     
==========================================
  Files           4        4              
  Lines         248      308      +60     
  Branches       28       44      +16     
==========================================
+ Hits          228      293      +65     
+ Misses         12        7       -5     
  Partials        8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…efore)

See astral-sh/ruff#9048

This is due to a change in constellation 1.2.3 - see vimc/orderly-web-deploy#56 for an analogous fix

I realised firstly that: - We need to be able to customise branding config in scenarios where packit-deploy does not provide the proxy, and its provided otherwise, as is the set-up in Montagu. Montagu handles the proxy in that case. - But running packit-deploy in a proxy-less mode doesn't in fact make knowing the app file locations complicated, since the Packit app Dockerfile specifies the location for the built app to be copied to, so we can rely on that to be the case for both proxy-less and non-proxy-less modes of packit-deploy.

https://github.com/mrc-ide/packit-deploy/blame/021e8b254d3c8a99de993ddcdebb89672cbb1b4a/src/packit_deploy/packit_constellation.py

…on brand name

…f basicauth

…randing branch

EmmaLRussell

Looks great! 😎

EmmaLRussell · 2025-05-22T12:29:54Z

README.md

@@ -117,3 +121,31 @@ docker exec -it packit-packit-db psql -U packituser -d packit
 ```

 If you have anything else running on port 80 or 443, nothing will work as expected; either stop that service or change the proxy port in the configuration that you are using.
+
+## Custom branding config


This section is really useful. Might be worth explicitly stating which yml values each of these features correspond to - or at least pointing to the relevant example config.

EmmaLRussell · 2025-05-22T12:31:16Z

README.md

+
+## Custom branding config
+
+Custom branding is disabled unless both a logo and brand name are configured, since we don't want to display an incorrect combination of brand name and logo/favicon.


Not sure about this - I feel like the brand name could be the minimum as you might want to just show the default logo if you want to give the app a project-centred name but don't have a bespoke logo.

Fair enough. I'll make it so all settings are optional.

EmmaLRussell · 2025-05-22T12:33:11Z

config/githubauth/packit.yml

@@ -42,10 +42,10 @@ outpack:
 packit:
  api:
    name: packit-api
-    tag: main
+    tag: mrc-6035-custom-branding # revert to main after mrc-6035-custom-branding is merged


I'm a bit surprised that you needed to update these tags for configurations which don't make use of custom branding..?

Yeah I don't remember why, I do remember needing them in one case and thinking what the hell I may as well put it everywhere.

EmmaLRussell · 2025-05-22T12:35:57Z

config/complete/packit.yml

+      accent: "hsl(0 100% 50%)"
+      accent_foreground: "hsl(123 100% 50%)"
+    dark:
+      accent: "hsl(30 100% 50%)"
+      accent_foreground: "hsl(322, 50%, 87%)"


For those who don't natively speak hsl, it might be nice to put comments describing each of these colours.

EmmaLRussell · 2025-05-22T12:38:16Z

config/complete/packit.yml

+
+## Branding configuration
+## If only one of 'name' or 'logo_path' is set, both will be ignored.
+brand:


I guess an alternative would have been to put each of these values under the key of the relevant container rather than having the top level "brand" key. I think it makes sense to keep them together, but I wonder if grouping them into "app" and "api" values here might be useful to clearly demarcate which values are applied in which container.

There is no such clear demarcation, since the logo file path is used by the configuration of both the app and the api.

There is a difference in which containers the deploy tool applies the values to - the app container uses the logo path, but it gets it from the api without any explicit tinkering from the deploy tool, which needs to provide the logo to the api.

But the user of the deployment isn't necessarily going to be thinking about it those terms (though they might be!) so fine as is.

EmmaLRussell · 2025-05-22T12:45:52Z

.github/workflows/test.yml

+          docker pull redis:5.0
+          docker pull mrcide/outpack.orderly:main
+          docker pull mrcide/outpack_server:main
+          docker pull mrcide/packit-db:main
+          docker pull mrcide/packit-api:main
+          docker pull mrcide/packit:main
+          docker pull mrcide/packit-proxy:main


This is for integration testing? You should be able to start the Packit constellation with the --pull flag. You might be pulling the wrong branches if you rely on this.

this is really a constellation issue that I need to fix at some point

EmmaLRussell · 2025-05-22T12:57:13Z

src/packit_deploy/config.py

@@ -75,6 +77,39 @@ def __init__(self, path, extra=None, options=None):
        else:
            self.proxy_enabled = False

+        self.branding_enabled = bool(dat.get("brand", {}).get("name") and dat.get("brand", {}).get("logo_path"))


This feels a little easier to reason about, though I assume the logic shakes down correctly with Python casting.

Suggested change

self.branding_enabled = bool(dat.get("brand", {}).get("name") and dat.get("brand", {}).get("logo_path"))

self.branding_enabled = bool(dat.get("brand", {}).get("name")) and bool(dat.get("brand", {}).get("logo_path"))

Also, it might be tidier to pull out the brand config once, before this line and then query that rather than query from the top level a bunch of times e.g. brand_config = dat.get("brand, {})

I've removed the whole concept of 'branding enabled/disabled'.

EmmaLRussell · 2025-05-22T13:13:21Z

src/packit_deploy/packit_constellation.py

+        try:
+            env.update({"PACKIT_BRAND_LOGO_LINK": cfg.brand_logo_link})
+        except AttributeError:
+            print("[packit-api] A brand logo link was not provided (optional)")


Why not just check if the attribute is set and not attempt to set the value if it isn't provided?

EmmaLRussell · 2025-05-22T13:15:44Z

src/packit_deploy/packit_constellation.py

+        env.update({"PACKIT_BRAND_LOGO_NAME": cfg.brand_logo_name})
+        env.update({"PACKIT_BRAND_LOGO_ALT_TEXT": cfg.brand_logo_alt_text})


env is just an ordinary dictionary so you could just set values like
env["PACKIT_BRAND_LOGO_NAME"] = cfg.brand_logo_name.

Or conversely you could take advantage of update and set both values at once:

Suggested change

env.update({"PACKIT_BRAND_LOGO_NAME": cfg.brand_logo_name})

env.update({"PACKIT_BRAND_LOGO_ALT_TEXT": cfg.brand_logo_alt_text})

env.update({

"PACKIT_BRAND_LOGO_NAME": cfg.brand_logo_name,

"PACKIT_BRAND_LOGO_ALT_TEXT": cfg.brand_logo_alt_text})

richfitz

This is looking good when run up; comments, questions and suggestions inline

richfitz · 2025-05-22T11:11:34Z

README.md

+
+### Logo (required)
+
+The logo file is bind-mounted into the front-end container, in a public folder, and the packit api has an env var set for the filename of the logo, so that it can tell the front end where to look for the file. Your logo file should be in the same directory as the config file.


What size should this be as a suggestion? Does it matter or is is just resized on the fly? Why is it required? Can we not fall back on the default version if it is not found?

What size should this be as a suggestion? Does it matter or is is just resized on the fly?

I'll put in a comment to say that the logo will be constrained to the height of the header and the width is kept proportional.

Why is it required?

Removed requirement.

Can we not fall back on the default version if it is not found?

Let's discuss over at #12 (comment)

richfitz · 2025-05-22T11:12:06Z

README.md

+
+The logo file is bind-mounted into the front-end container, in a public folder, and the packit api has an env var set for the filename of the logo, so that it can tell the front end where to look for the file. Your logo file should be in the same directory as the config file.
+
+### Logo alt text (optional)


Should this be alt text or title text? Should it be possible to set both?

Yes, those are separately configurable. Title text is referred to as 'brand name'.

richfitz · 2025-05-22T11:12:25Z

README.md

+
+### Logo link (optional)
+
+This is to allow a configurable link destination for when the user clicks the logo. In VIMC's case this would be a link back to Montagu. This is set as an env var in the packit api, which passes it on to the front end.


Does the fact that these come through as an envvar matter for the README?

Dunno, it might be useful documentation, but the problem with documentation is that it gets out of sync with reality. @EmmaLRussell you are someone who has found this readme useful, what do you reckon?

I think a bit of docs on the mechanics of how these values work is useful once it's fairly stable, as it's quite bitty and easy to forget which values go where ..

richfitz · 2025-05-22T11:12:55Z

README.md

+
+### Brand colors (optional)
+
+The brand colors are written as css variables into the public custom.css file, which override default variables in the front-end. If no colors are provided for the dark theme, the light theme colors are reused.


List colours that can be modified here?

richfitz · 2025-05-22T11:32:06Z

src/packit_deploy/packit_constellation.py

+        # We configure the title tag of the index.html file here, rather than updating it dynamically with JS,
+        # since using JS results in the page title visibly changing a number of seconds after the initial page load.
+        substitute_file_content(
+            container, f"{cfg.app_html_root}/index.html", r"(?<=<title>).*?(?=</title>)", cfg.brand_name


https://stackoverflow.com/a/1732454

https://xkcd.com/1638/

richfitz · 2025-05-22T11:37:42Z

src/packit_deploy/packit_constellation.py

+            f"  --custom-accent-foreground: {cfg.brand_accent_foreground_dark};\n"
+            "}\n"
+        )
+        substitute_file_content(container, f"{cfg.app_html_root}/css/custom.css", r".*", new_css, flags=re.DOTALL)


is this just (over)writing a file? Can we do this more obviously?

It is. I can make the fact it's doing this more obvious by wrapping substitute_file_content with a method overwrite_file. It's somewhat convenient (for developers of packit) to have a file custom.css in the packit repo, which explains in comments where the custom css variables will come from in production, and which they can use to play with setting custom variables.

richfitz · 2025-05-22T11:40:11Z

src/packit_deploy/packit_constellation.py

+        substitute_file_content(
+            container, f"{cfg.app_html_root}/index.html", r"(?<=<title>).*?(?=</title>)", cfg.brand_name
+        )
+        substitute_file_content(container, f"{cfg.app_html_root}/index.html", r"favicon\.ico", cfg.brand_favicon_name)


I'm confused on this, and perhaps generally, why the index.html needs writing at all. Can we not have a path to the content that never changes (say /favicon.ico) and then have the backend try first (say) custom/favicon.ico then fall back on baked-in/favicon.ico for example? With the same strategy for other resources.

I want to have configurable favicon filename so that people can use any type of file, which might require a different extension than .ico.

With regards to other resources - of which there is one, the logo - the same argument could apply. It's also useful to have the logo file be named whatever, so that you can maintain an obvious relationship between it and the file in your organisation's perhaps-chaotic dropbox folder of various different logo variants and versions.

Also document colours

richfitz · 2025-05-23T09:50:36Z

config/basicauthcustombrand/packit.yml

+      # Secret used to generate JWT tokens - this can be any string, the secret at this key in the vault is a random
+      # 32 char string, and is probably fine to re-use


Suggested change

# Secret used to generate JWT tokens - this can be any string, the secret at this key in the vault is a random

# 32 char string, and is probably fine to re-use

# Secret used to generate JWT tokens - this can be any string

david-mears-2 force-pushed the custom-branding branch from 429f1c0 to df68de1 Compare May 2, 2025 13:09

david-mears-2 force-pushed the custom-branding branch from c0b6ff7 to 5ab8c93 Compare May 9, 2025 17:03

david-mears-2 mentioned this pull request May 12, 2025

Custom branding [packit] mrc-ide/packit#202

Open

david-mears-2 added 26 commits May 16, 2025 14:29

WIP custom branding

18f9e8e

Update gha versions and remove end-of-life python versions

eefc330

Get Ruff working again (don't know why it was broken or ever worked b…

d6518ea

…efore)

Apply ruff formatting

6e8cfb7

Use ruff linting and formatting - these are different

adab3b4

See astral-sh/ruff#9048

Explicitly pull docker images

c4002ac

This is due to a change in constellation 1.2.3 - see vimc/orderly-web-deploy#56 for an analogous fix

Fix integration test and vault deprecation warning

a007019

Update config test to use 'complete' config

8f619d8

Update README.md

c291be7

Update tests

06e6b2b

Rename example favicon.ico for more informative tests

6516588

Fix integration test

01d175e

Update gha versions

36040b4

Use favicon's actual filename

65d5149

Add example config basicauthcustombrand

30f09c7

Remove comment

a060279

https://github.com/mrc-ide/packit-deploy/blame/021e8b254d3c8a99de993ddcdebb89672cbb1b4a/src/packit_deploy/packit_constellation.py

Formatting

0d94e52

Inject CSS variables into front-end container

2c845b7

Refactor interface of substitute_file_content to mirror re.sub

b089d90

Update README.md

8c56dba

Don't set brand name as api env var; set default logo alt text based …

05fa8f1

…on brand name

Update README.md

c9c6016

Copy changes to basicauth config from PR 13 to custom brand variant o…

9c98c0b

…f basicauth

Use VIMC colors in example brand config, and point config at packit b…

a794820

…randing branch

Point all configs to feature branch in order to pass tests (validly)

d200f4f

david-mears-2 added 2 commits May 16, 2025 14:33

Lint

2a85114

Update integration tests

95fdd41

david-mears-2 force-pushed the custom-branding branch from e15a53d to 95fdd41 Compare May 16, 2025 13:33

david-mears-2 changed the title ~~WIP custom branding~~ Custom branding May 16, 2025

Reformat README

aba6b46

david-mears-2 marked this pull request as ready for review May 16, 2025 14:34

david-mears-2 requested a review from EmmaLRussell May 16, 2025 14:35

david-mears-2 changed the title ~~Custom branding~~ Custom branding [packit-deploy] May 16, 2025

david-mears-2 requested a review from richfitz May 19, 2025 11:11

EmmaLRussell reviewed May 22, 2025

View reviewed changes

richfitz requested changes May 22, 2025

View reviewed changes

david-mears-2 force-pushed the custom-branding branch from e48c7bc to 045ca04 Compare May 22, 2025 15:53

Make all custom branding settings optional

61eec93

Also document colours

david-mears-2 force-pushed the custom-branding branch from 045ca04 to 61eec93 Compare May 22, 2025 16:03

david-mears-2 added 3 commits May 22, 2025 17:27

Update documentation

cf61538

Prevent AttributeError if branding config attrs are unset

80bbac3

Use 'overwrite_file' method to wrap substitute_file_content method

d87bdff

david-mears-2 requested review from richfitz and EmmaLRussell May 22, 2025 16:44

richfitz approved these changes May 27, 2025

View reviewed changes

EmmaLRussell approved these changes May 27, 2025

View reviewed changes

richfitz merged commit 5cfc2e4 into main May 30, 2025
8 checks passed


		## Custom branding config

		Custom branding is disabled unless both a logo and brand name are configured, since we don't want to display an incorrect combination of brand name and logo/favicon.

	self.branding_enabled = bool(dat.get("brand", {}).get("name") and dat.get("brand", {}).get("logo_path"))
	self.branding_enabled = bool(dat.get("brand", {}).get("name")) and bool(dat.get("brand", {}).get("logo_path"))

		env.update({"PACKIT_BRAND_LOGO_NAME": cfg.brand_logo_name})
		env.update({"PACKIT_BRAND_LOGO_ALT_TEXT": cfg.brand_logo_alt_text})


		### Logo (required)

		The logo file is bind-mounted into the front-end container, in a public folder, and the packit api has an env var set for the filename of the logo, so that it can tell the front end where to look for the file. Your logo file should be in the same directory as the config file.


		The logo file is bind-mounted into the front-end container, in a public folder, and the packit api has an env var set for the filename of the logo, so that it can tell the front end where to look for the file. Your logo file should be in the same directory as the config file.

		### Logo alt text (optional)


		### Logo link (optional)

		This is to allow a configurable link destination for when the user clicks the logo. In VIMC's case this would be a link back to Montagu. This is set as an env var in the packit api, which passes it on to the front end.


		### Brand colors (optional)

		The brand colors are written as css variables into the public custom.css file, which override default variables in the front-end. If no colors are provided for the dark theme, the light theme colors are reused.

		# Secret used to generate JWT tokens - this can be any string, the secret at this key in the vault is a random
		# 32 char string, and is probably fine to re-use

Custom branding [packit-deploy] #12

Custom branding [packit-deploy] #12

Uh oh!

Conversation

david-mears-2 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How custom branding works

Test this branch locally

Other changes

Before merging

Uh oh!

codecov bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

EmmaLRussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richfitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-mears-2 May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

david-mears-2 commented May 1, 2025 •

edited

Loading

codecov bot commented May 2, 2025 •

edited

Loading

david-mears-2 May 22, 2025 •

edited

Loading

david-mears-2 May 22, 2025 •

edited

Loading