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.

Sign up for GitHub

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

Jump to bottom

Simplify deployments and update to new production buckets #11649

Merged
merged 3 commits into from
Dec 1, 2023
Merged

Simplify deployments and update to new production buckets #11649

merged 3 commits into from
Dec 1, 2023

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Nov 22, 2023

  1. Remove all custom AWS code and replace with aws cli usage
  2. No longer gzip everything manually, let AWS compress automatically for us.
  3. Remove no longer needed npm modules

@mramato mramato requested a review from ggetz November 22, 2023 22:55
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ Signed CLA found.
  • ❔ CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@mramato mramato marked this pull request as draft November 22, 2023 23:01
@mramato
Copy link
Contributor Author

mramato commented Nov 22, 2023

Just realized I missed a few things, will bump this PR when ready for review.

@mramato mramato changed the title Simplify deployments Simplify deployments and update to new production buckets Nov 22, 2023
@mramato
Simplify deployments
310dc31 
1. Remove all custom AWS code and replace with aws cli usage
2. No longer gzip everything manually, let AWS compress
automatically for us.
3. Remove no longer needed npm modules
@mramato mramato marked this pull request as ready for review November 23, 2023 00:55
@mramato
Copy link
Contributor Author

mramato commented Nov 23, 2023

@ggetz this is ready.

I mainly matched what was already there but I think this will make it way easier to change going forward. I think there is a conversation to be had over exactly what we should deploy for CI and what we shouldn't, but I didn't want to expand the scope of this PR.

I also noticed the deployment/zip links are broken across all PRs (I'll see if I can put a small change for that and open a separate PR for it). If you fix the links manually based on the above checks, they are all correct and work.

@mramato
Fix deploy URLs
57df63e 
We were adding `index.html` to the deploy URL which made the
derived URLs incorrect. Since index.html isn't needed, I just
removed it and now deploy URLs should be fixed
@mramato
Copy link
Contributor Author

mramato commented Nov 23, 2023

I also noticed the deployment/zip links are broken across all PRs (I'll see if I can put a small change for that and open a separate PR for it). If you fix the links manually based on the above checks, they are all correct and work.

This turned out to be a 1-liner so I just pushed it here. See 57df63e

@mramato mramato requested a review from jjhembd November 27, 2023 13:56
@ggetz
Copy link
Contributor

ggetz commented Dec 1, 2023 •
edited
Loading

Thanks @mramato! I've confirmed the dev deployments are working as expected.

I have a question above below related to the production deployment.

ggetz
ggetz reviewed Dec 1, 2023
View reviewed changes
gulpfile.js Show resolved Hide resolved
.github/workflows/dev.yml Show resolved Hide resolved
.github/workflows/prod.yml
run: |
unzip Cesium-$(cat package.json | jq -r '.version').zip -d Build/release/ -x "Apps"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is jq built in or does it need to be installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've used it in other workflows without doing anything special so I'm confident it's built in.

@mramato
Changes after review
1d426cb 
* Remove unused `mime` package
* Don't deploy `Build/Specs/e2e`
@mramato
Copy link
Contributor Author

mramato commented Dec 1, 2023

@ggetz updated and back to you.

@ggetz
Copy link
Contributor

ggetz commented Dec 1, 2023

Great, thanks @mramato! I will merge once CI finishes.

@ggetz ggetz merged commit 03dd717 into main Dec 1, 2023
5 checks passed
@ggetz ggetz deleted the update-deployment branch December 1, 2023 16:00
@ggetz ggetz mentioned this pull request Dec 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggetz ggetz ggetz left review comments

@jjhembd jjhembd Awaiting requested review from jjhembd

Assignees
No one assigned
Labels
priority - next release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mramato @cesium-concierge @ggetz