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

ci: Move static builds to d2l.dev bucket #3898

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

eKoopmans
Copy link
Contributor

@eKoopmans eKoopmans commented Aug 17, 2023

This moves core's static builds to the d2l.dev S3 bucket, as part of standardizing the PR Preview process. This will let us delete the custom brightspace-ui-core.d2l.dev bucket.

This is step 2 of a multi-step process for UI core:

  1. repo-settings: Get live.d2l.dev S3 access
  2. BrightspaceUI/core: Switch core's CI and Release to publish to live.d2l.dev
  3. BrightspaceUI/core: In all open PRs, delete the "PR Preview" comment and merge main to re-enable previews
  4. d2l.dev: Remove all brightspace-ui-core definitions! 🎉
  5. repo-settings: Remove the assumable role brightspace-ui-core.d2l.dev-githubactions-access
  6. AWS console: Delete the brightspace-ui-core.d2l.dev bucket and IAM roles
    • I'll need help with this last step, I don't have access to the relevant account anymore

US156216

@eKoopmans eKoopmans requested a review from a team as a code owner August 17, 2023 14:03
@eKoopmans
Copy link
Contributor Author

eKoopmans commented Aug 17, 2023

The publish step will fail until https://github.com/Brightspace/repo-settings/pull/1250 is merged.

Publish is working!

role-duration-seconds: 3600
aws-region: ca-central-1

- name: Publish
uses: BrightspaceUI/actions/publish-to-s3@main
with:
bucket-path: s3://brightspace-ui-core.d2l.dev/branches/pr-${{ github.event.number }}
bucket-path: s3://d2l.dev/pr-previews/brightspace-ui-core/pr-${{ github.event.number }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I pictured this as:
bucket-path: s3://d2l.dev/pr-previews/BrightspaceUI/core/pr-${{ github.event.number }}

But I guess it doesn't really matter, as long as there are no collisions in what people choose 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

If we did do org/repo, we could actually add a special thing to repo_settings so someone could do:

pr_previews: true

and under the hood it would setup the d2ldev piece. But that doesn't work with the "main" site piece.

Just spit-balling here - what are your thoughts on this:

  • All previews go into d2l.dev/pr-previews
    • pr-previews/main/<org>/<repo>/ for the main site that doesn't expire
    • pr-previews/prs/<org>/<repo>/pr-<#> for the PR sites
  • The expiry prefix is changed to pr-previews/prs
  • For now, someone sets this in repo-settings (need to confirm this doesn't blow up):
    d2ldev:
      s3:
        - subdomain: pr-previews
          path: main/<org>/<repo>
        - subdomain: pr-previews
          path: prs/<org>/<repo>
    
    • But we eventually switch this to pr_previews: true and set it for them
  • If someone wants a fancy path like brightspace-ui-core.d2l.dev, they can set it up in d2l.dev. But they don't actually have to set up anything in d2l.dev if they're fine with https://pr-previews.d2l.dev/prs/BrightspaceUI/core/pr-${{ github.event.number }}/ and https://pr-previews.d2l.dev/main/BrightspaceUI/core/

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'm open to that, except that I think the phrase "PR preview" doesn't apply any more to the "main" static builds.

What about calling them all repo-demos or code-demos?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah good with either of those! Or even demos could work, if we want to try to make the default urls as easy to remember as possible (maybe not useful with url autocomplete lol)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I might be back to being against this, mostly because of how the URLs would work:

I don't like that folder restriction, but maybe it's fine...

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't the pr and main need to be at the beginning to be able to set the retention properly, since we can't use wildcards in the prefix? Like https://demos.d2l.dev/main/BrightspaceUI/core/ (or I think we could get away with https://demos.d2l.dev/BrightspaceUI/core/) and https://demos.d2l.dev/prs/BrightspaceUI/core/

I guess we could map it in some way with the routes, but this way maybe alleviates your concern cause there's no folder name they need to avoid. They publish their main site to d2ldev/demos/<org>/<repo> and their PR sites to d2ldev/demos/prs/<org>/<repo>/<whatever-they-want-really>-<pr_num>. And the lifecycle prefix is "/demos/prs/"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the prefix would need to be there in the bucket, but like you said the routing doesn't need to match the bucket.

I like keeping the main demo link clean as https://demos.d2l.dev/BrightspaceUI/core/. You're right that sticking the prs prefix into the URL works, though it's a bit buried there...

We could also just do a completely different route and serve the prs/ bucket up at https://pr-previews.d2l.dev/BrightspaceUI/core/. That might be an anti-pattern though, d2l.dev is generally set up so the bucket prefix matches the subdomain.

So then - why not just two separate bucket prefixes? Keep pr-previews as it currently is, and just add one for demos? Is there any benefit to trying to merge them into one, apart from their content being similar? Keeping pr-previews clearly separate might also make the expiration rule more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I thought on it a bit, I can get behind https://demos.d2l.dev/prs/BrightspaceUI/core/ - the URL for these aren't too important anyway, since you're only going to be visiting them as links from PRs. And for that same reason, having a whole subdomain dedicated to them isn't too useful.

Any other thoughts before I go configure that in d2l.dev?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with either of your last two comments - going with https://demos.d2l.dev/prs/BrightspaceUI/core/, or just making two bucket prefixes to keep things clear, since we could still wrap that in repo-settings behind a single command. I was trying to think of two buckets prefixes that worked nicely together lol... demo and demo-prs? pr-preview and repo-preview? preview and pr-preview?

@eKoopmans
Copy link
Contributor Author

Good news: I've updated this to use live.d2l.dev!

Bad news: It's failing... 😞

https://github.com/BrightspaceUI/core/actions/runs/5965826665/job/16184501505?pr=3898#step:8:93

upload failed: build/components/alert/alert.js to s3://d2l.dev/live/prs/BrightspaceUI/core/pr-3898/components/alert/alert.js An error occurred (AccessDenied) when calling the PutObject operation: Access Denied

I don't know why... the role should have access to s3://d2l.dev with the prefix live/prs/BrightspaceUI/core, from https://github.com/Brightspace/repo-settings/pull/1261. My only thoughts right now are:

  • maybe the permissions take longer to update? (though I definitely waited until after CD on repo-settings had finished)
  • maybe something to do with capital letters in the prefix/S3 route?

@svanherk
Copy link
Contributor

maybe the permissions take longer to update? (though I definitely waited until after CD on repo-settings had finished)

You can check when the tokens last rotated here: https://tokens.d2l.dev/status
So could be that, I didn't do the timezone math lol

@eKoopmans
Copy link
Contributor Author

Very cool, thanks Stacey! I think that's it, it last updated at 11:10 eastern, and the job finished 11:21.

@BrightspaceUI BrightspaceUI deleted a comment from github-actions bot Aug 24, 2023
@github-actions
Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-3898/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@eKoopmans
Copy link
Contributor Author

eKoopmans commented Aug 24, 2023

Okay we're in business! 🚀

(The fix was https://github.com/Brightspace/d2l.dev/pull/4637 )

Copy link
Contributor

@svanherk svanherk left a comment

Choose a reason for hiding this comment

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

Yay! Thanks for all the work this ended up being @eKoopmans! 😅

role-duration-seconds: 3600
aws-region: ca-central-1

- name: Publish
uses: BrightspaceUI/actions/publish-to-s3@main
with:
bucket-path: s3://brightspace-ui-core.d2l.dev/branches/pr-${{ github.event.number }}
bucket-path: s3://d2l.dev/live/prs/BrightspaceUI/core/pr-${{ github.event.number }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider making this a little more copy-pastable

Suggested change
bucket-path: s3://d2l.dev/live/prs/BrightspaceUI/core/pr-${{ github.event.number }}
bucket-path: s3://d2l.dev/live/prs/${{ github.repository }}/pr-${{ github.event.number }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call Owen, I've updated the docs to use {{ github.repository }} too!

@eKoopmans eKoopmans merged commit bcad50e into main Aug 24, 2023
6 checks passed
@eKoopmans eKoopmans deleted the ekoopmans/pr-previews-d2ldev branch August 24, 2023 22:40
@eKoopmans
Copy link
Contributor Author

🎉

image

@ghost
Copy link

ghost commented Aug 29, 2023

🎉 This PR is included in version 2.141.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants