Skip to content
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

Update <link> static assets to use the new domain #8916

Merged
merged 8 commits into from
Mar 3, 2021

Conversation

jimjohnsonrollings
Copy link
Contributor

@jimjohnsonrollings jimjohnsonrollings commented Feb 24, 2021

Resolves #8915

Overall change:
Updates the URL used by tags in all pages to point to new URLs for those assets.

Code changes:

  • Update URL used
  • Update Test
  • Update 486 snapshots
  • Update readme to replace a reference to npm with yarn

  • I have assigned myself to this PR and the corresponding issues
  • I have added the cross-team label to this PR if it requires visibility across World Service teams
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)
  • This PR requires manual testing

@jimjohnsonrollings jimjohnsonrollings self-assigned this Feb 24, 2021
@jimjohnsonrollings jimjohnsonrollings added ws-articles Tasks for the WS Articles Team cross-team For visibility for both World Service teams (Engage & Media) and removed ws-articles Tasks for the WS Articles Team labels Feb 24, 2021
sareh
sareh previously requested changes Feb 25, 2021
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, it'd be good to ensure the headers for cache-control max-age are set to the correct length and define what value the strict-transport-security: max-age value should be set too, and update that, before making this change.

sizes: '96x96',
},
{
href:
'https://news.files.bbci.co.uk/include/articles/public/news/images/icons/icon-128x128.png',
'https://static.files.bbci.co.uk/ws/simorgh-assets/public/news/images/icons/icon-128x128.png',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache-control header max-age is different
Old endpoint:
curl -Is https://static.files.bbci.co.uk/ws/simorgh-assets/public/news/images/icons/icon-128x128.png
cache-control: max-age=31622400 (366 days)

New endpoint:
curl -Is https://news.files.bbci.co.uk/include/articles/public/news/images/icons/icon-128x128.png
cache-control: max-age=604800 (7 days)
and a new header:
strict-transport-security: max-age=2592000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache control change is I think deliberate - we didn't want so long on the new one so that we can more easily update the images, and settled on seven days as reasonably short without being too short.

I don't know about the other one, but maybe @chris-hinds or @jamesdonoh do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. To enable updates to logo files, I'd suggest a better model would be to include a hash of the file content in the filename and keep a long cache-control max-age.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks.

Given this PR is just updating this URL to be consistent with the ones already in the manifest files, would it be okay to proceed with this as is, and I can write a new issue to cover considering the caching approaches?

cc @joshcoventry

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as the team is happy with the cache hit going from 366 days to 7, sure.
Regarding the new header strict-transport-security: max-age=2592000 (30 days) - this tells the browser to use HTTPS instead of HTTP for 30 days. This could also be extended to 366 days, since I don't believe this is expected to change.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache control change is I think deliberate - we didn't want so long on the new one so that we can more easily update the images, and settled on seven days as reasonably short without being too short.

I don't know about the other one, but maybe @chris-hinds or @jamesdonoh do?

Apologies have only just seen this. Yes the cache changes were deliberate to ensure that we have a reasonable cache time but are able to still update the images.

Setting a hash in the url is a good idea but something we'd have to work on when the team has more time.

Regarding setting strict-transport-security we should work with the Mozart team on this and perhaps this is a default header that should be applied if it isn't already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per what Chris has said, happy for this PR to go out as is - we can pick up other work relating to the cache afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've drafted an issue here for the caching stuff: #8931

@chris-hinds chris-hinds dismissed sareh’s stale review March 2, 2021 14:58

This work will be done as part of another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team For visibility for both World Service teams (Engage & Media)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<link> elements on all pages use old static asset paths
4 participants