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

feat: Add prefixStagePaths parameter to aws-s3 deployments #1015

Merged
merged 7 commits into from
Mar 7, 2023

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented Feb 14, 2023

Note
Easiest to review commit by commit as this change extracts some existing behaviour into a trait - this specific change should be considered a no-op.

Background

In the aws-s3 deployment type, the path within S3 that we upload to is controlled by the following parameters:

  • prefixPackage
  • prefixStack
  • prefixApp
  • prefixStage

This results in the path being the same regardless of the stage being deployed.

For example, for the file image.jpg and a shared bucket the upload path will be something like <bucket>/<STAGE>/image.jpg. If the bucket was not shared across stages (as recommended) the upload path will be something like <bucket>/image.jpg.

What does this change?

Introduces a new prefixStagePaths parameter for the aws-s3 deployment type.

This is to support services such as https://github.com/guardian/interactive-atom-maker. interactive-atom-maker is configured in a slightly non-standard way - CODE files live in <bucket>/atoms-CODE/image.jpg, PROD files live in <bucket>/atoms/image.jpg1. The behaviour described above doesn't quite work here.

This change allows us to do:

prefixStagePaths:
  - CODE: atoms-CODE
  - PROD: atoms

When set, prefixStagePaths takes precedence over all other prefix<App|Stage|etc...> options.

How to test

Unit tests have been added to check:

  • When prefixStagePaths is not set, we maintain existing behaviours of prefixApp, prefixStage, etc.
  • What happens when prefixStagePaths is set
  • When prefixStagePaths is set, describing stages of CODE and PROD, what happens when deploying stage of UAT (it should fail)
  • When prefixStagePaths and prefix* are both set, what wins

For a practical demonstration of this new parameter, see https://github.com/guardian/csti-interactives/pull/53. To test, deploy this branch to CODE, then deploy a build from https://github.com/guardian/csti-interactives/pull/53, and observe the paths which Riff-Raff uploads files to.

How can we measure success?

https://github.com/guardian/csti-interactives (specifically https://github.com/guardian/csti-interactives/pull/53) is able to use Riff-Raff for continuous delivery.

Footnotes

  1. We could update how interactive-atom-maker works, however this would involve updating all the articles that reference the files in S3, which is non-trivial.

@twrichards
Copy link
Contributor

twrichards commented Feb 27, 2023

lol, I think I've just raised a very similar feature in #1033 without seeing this

EDIT: this is nicer, so closing #1033

@twrichards
Copy link
Contributor

When might this PR get finished / merged? I'd like to use it for guardian/actions-static-site#19 (comment)

Happy to help. Is it just tests which need adding?

@akash1810 akash1810 force-pushed the s3-prefixStagePaths branch 3 times, most recently from 6499dae to 8fac27d Compare March 2, 2023 09:30
@ob6160 ob6160 force-pushed the s3-prefixStagePaths branch from bcf6c62 to 0ea0b30 Compare March 2, 2023 16:01
@ob6160 ob6160 requested a review from joecowton1 March 2, 2023 17:05
@ob6160 ob6160 marked this pull request as ready for review March 2, 2023 17:24
@ob6160 ob6160 requested review from a team as code owners March 2, 2023 17:24
prefixStagePaths(pkg, target, reporter)

if (prefixFromStagePaths.isEmpty) {
S3Upload.prefixGenerator(
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a little strange that some of the logic for generating a prefix is in S3Upload.prefixGenerator and some of it here, should this be consolidated?

Copy link
Member Author

@akash1810 akash1810 Mar 6, 2023

Choose a reason for hiding this comment

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

S3Upload.prefixGenerator is used in a few places. Suggest we further refactor/DRY out in a follow-up PR to keep the diff in this PR to a minimum. WDYT?

def name = "n/a"
}

class S3ObjectPrefixParametersTest extends AnyFlatSpec with Matchers {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 well tested, very nice.

twrichards added a commit to guardian/actions-static-site that referenced this pull request Mar 3, 2023
@twrichards twrichards changed the title feat: Add prefixStagePaths parameter to aws-s3 deployments feat: Add prefixFromStagePaths parameter to aws-s3 deployments Mar 3, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Mar 3, 2023
@akash1810 akash1810 changed the title feat: Add prefixFromStagePaths parameter to aws-s3 deployments feat: Add prefixStagePaths parameter to aws-s3 deployments Mar 7, 2023
@akash1810 akash1810 force-pushed the s3-prefixStagePaths branch from 22d231d to 934cbba Compare March 7, 2023 06:50
@akash1810 akash1810 enabled auto-merge March 7, 2023 06:56
@akash1810 akash1810 disabled auto-merge March 7, 2023 06:57
The parameter that can be set in `riff-raff.yaml` is `prefixStagePaths`, `prefixFromStagePaths` is a local variable name.
@akash1810 akash1810 enabled auto-merge March 7, 2023 07:09
@akash1810 akash1810 merged commit 4e0d1f2 into main Mar 7, 2023
@akash1810 akash1810 deleted the s3-prefixStagePaths branch March 7, 2023 11:06
twrichards added a commit to guardian/actions-static-site that referenced this pull request Mar 7, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Mar 31, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Mar 31, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Apr 17, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Apr 17, 2023
twrichards added a commit to guardian/actions-static-site that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants