ci(aio): change AIO preview server stuff to use CircleCI by petebacondarwin · Pull Request #23576 · angular/angular

petebacondarwin · 2018-04-27T13:56:59Z

The system now waits for a CircleCI webhook to tell the upload server that a build has completed.
The upload server then pulls down the build artifact and passes it along as before to the rest of the preview creation logic.

This means that there is no need to store a secret on CircleCI for pushing the artifacts to the upload server; which means that we no longer rely upon JWT.

The downloaded artifact is stored in a temp location; this is cleared out along with the actual preview builds by the periodic cleanup task.

There are some (self-serving) changes to the testing of components that were not significantly changed for this PR. This includes more use of async/await for managing async tests; and also making some API public (rather than protected). This allows us to unit test, while maintaining type safety. Previously, we had to cast to any which meant that changes to the API may not be passed through to the unit tests.

TODO:

check for CI success before processing
consider not downloading an artifact if it has been downloaded already
fix up / write more e2e tests
update the docs
consider renaming "upload server" to "preview server"
configure the rest of the environment variables for the new processing
ascertain whether we need a CircleCI access token for making the GET requests
work out how (and when) to run the PWA-score test; probably by the preview-server triggering a specific CircleCI job via API

IgorMinar

at a high level this looks good. I'll let @gkalpak take a more thorough look through all the changes.

IgorMinar · 2018-04-27T14:03:10Z

aio/scripts/build-artifacts.sh

can we call this file with a more specific name? maybe aio-snapshot.tgz?

How about aio/aio-snapshot.tgz?

gkalpak

Half-way through (mostly reviewed anything outside scripts-js/).

gkalpak · 2018-05-09T12:49:52Z

.circleci/config.yml

AFAICT, destination needs to be kept in sync with AIO_ARTIFACT_PATH in preview server's Dockerfile. It is a good idea to add a comment about that too 😁

gkalpak · 2018-05-09T12:52:09Z

aio/aio-builds-setup/dockerbuild/Dockerfile

Why this change?

This is because we want to test failures where the artifact is too large. Rather than having to create an enormous file, I just set this a "bit" lower :-)

But why set it a "bit" lower (for tests)?

I don't understand the question. I set it lower because in the tests we now actually create a mock file that is returned from the mock CircleCI API. To test that the max-size check is working we would have to create a mock file that is larger than 20971520 bytes. What is the point of that when we can just set the limit to something smaller?

gkalpak · 2018-05-09T12:55:21Z

aio/aio-builds-setup/dockerbuild/Dockerfile

Not alphabetical order 😭

gkalpak · 2018-05-09T12:57:02Z

aio/aio-builds-setup/dockerbuild/Dockerfile

could we jump to v10 instead?

I'll see if we can

It looks like the JS tests all pass so we should be fine with Node 10

gkalpak · 2018-05-09T13:00:48Z

aio/aio-builds-setup/docs/_TOC.md

🙅‍♂️

Still no good 🙅‍♂️

gkalpak · 2018-05-09T17:31:16Z

aio/aio-builds-setup/dockerbuild/scripts-sh/upload-server-test.sh

Note: These files are made executable on the preview server. So there is no need to have them executable here (unless you want to execute them outside of the docker container...which you probably shouldn't be doing 😛).

gkalpak · 2018-05-09T17:34:08Z

aio/aio-builds-setup/dockerbuild/scripts-sh/clean-up.sh

This shouldn't be necessary?

gkalpak · 2018-05-09T17:35:53Z

aio/aio-builds-setup/dockerbuild/scripts-sh/test-env.sh

These new scripts (dev.sh, run-tests.sh, test-env.sh) should be documented in overview--scripts-and-commands.md.

Are you sure?

They are actually documented in misc--debug-docker-container.md. I'll add a bit to overview--scripts-and-commands.md for good measure.

gkalpak · 2018-05-09T17:36:52Z

aio/aio-builds-setup/dockerbuild/scripts-sh/run-tests.sh

I feel ~~a little~~very uncomfortable having this on the production server 😱

Still feeling uncomfortable.
As discussed, we could have these scripts in a directory that is not included in the docker container by default, but could be explicitly attached (e.g. as a volume).

This file no longer exists. It has been replaced by verify-setup-and-log, which no longer deletes anything:

aio-verify-setup ls -t /var/log/aio/upload-server-verify* | head -1 | xargs cat

gkalpak · 2018-05-09T17:38:58Z

aio/aio-builds-setup/dockerbuild/scripts-sh/dev.sh

What is this script? 😱 😱 😱

petebacondarwin · 2018-05-10T12:57:46Z

@gkalpak I have addressed yourr comments and reordered the commits. But I still have some tidying up to do in the TypeScript files - later today.

petebacondarwin · 2018-05-11T09:30:14Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/clean-up/build-cleaner.ts

So the key change here is that we now must also clean the downloads folder, which is populated with artifacts downloaded from CircleCI.

petebacondarwin · 2018-05-11T09:34:11Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/github-api.ts

Note that the specific GitHub API classes (like Teams and Pull Requests) now use this helper class by composition rather than inheritance. Therefore I had to make getPaginated a public method to expose it to these classes.

Why using composition over inheritance?

Composition provides a number of benefits. We only need to create on instance of the underlying GithubApi service, we can test that service independently of the subclasses that use it; and the clients of this service get a clean public API to work against.

petebacondarwin · 2018-05-11T09:36:44Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/common/utils.ts

Throwing and catching is simply to capture the stack trace to make debugging easier.

petebacondarwin · 2018-05-11T09:55:01Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/index-preverify-pr.ts

I wonder if we need to use a Dependency Injection framework here? :-P

petebacondarwin · 2018-05-11T09:58:50Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts

I converted these methods to static because really creating an instance of this class was unnecessary - there is not state in the class. The only reason I didn't pull them out into independent exported functions is that it is nice to have them in a container so that it is simple to spy/mock them for testing.

petebacondarwin · 2018-05-11T09:59:30Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts

Here is the meat of the change to this file - the new handler for the circle-build request.

petebacondarwin · 2018-05-11T10:00:23Z

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/upload-server-factory.ts

I don't believe I made any substantive change to the pr-updated request handler, other than a sprinkling of await.

mary-poppins · 2018-05-11T15:54:47Z

You can preview 2c4fc68 at https://pr23576-2c4fc68.ngbuilds.io/.

The server no longer has files uploaded to it. Instead it is more accurate to refer to it as dealing with "previews" of PRs.

Related discussion: angular#23576 (comment).

…anges Mostly (but not exclusively) a follow-up to angular#23576.

Related discussion: #23576 (comment). PR Close #25671

…anges (#25671) Mostly (but not exclusively) a follow-up to #23576. PR Close #25671

Related discussion: #23576 (comment). PR Close #25671

…anges (#25671) Mostly (but not exclusively) a follow-up to #23576. PR Close #25671

angular-automatic-lock-bot · 2019-09-14T00:22:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

petebacondarwin added state: WIP area: build & ci Related the build and CI infrastructure of the project comp: aio labels Apr 27, 2018

petebacondarwin requested a review from gkalpak April 27, 2018 13:56

googlebot added the cla: yes label Apr 27, 2018

petebacondarwin force-pushed the aio-circleci-builds branch from d9fe1fa to 7b4459a Compare April 27, 2018 13:57

IgorMinar reviewed Apr 27, 2018

View reviewed changes

petebacondarwin force-pushed the aio-circleci-builds branch from c068ebd to a5a8942 Compare April 27, 2018 16:21

petebacondarwin force-pushed the aio-circleci-builds branch 2 times, most recently from 439f2d1 to d50ffdc Compare May 9, 2018 17:43

gkalpak reviewed May 9, 2018

View reviewed changes

petebacondarwin force-pushed the aio-circleci-builds branch from d50ffdc to ffb223d Compare May 10, 2018 12:57

petebacondarwin commented May 11, 2018

View reviewed changes

petebacondarwin force-pushed the aio-circleci-builds branch from ffb223d to a91bb05 Compare May 11, 2018 09:40

petebacondarwin commented May 11, 2018

View reviewed changes

petebacondarwin force-pushed the aio-circleci-builds branch 5 times, most recently from c0012b4 to 2c4fc68 Compare May 11, 2018 14:55

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels May 11, 2018

IgorMinar added the state: WIP label May 11, 2018

petebacondarwin added 3 commits August 16, 2018 10:26

ci(docs-infra): add explicit return types to methods

d604ef7

ci(docs-infra): rename 'upload-server' to 'preview-server'

68bfe68

The server no longer has files uploaded to it. Instead it is more accurate to refer to it as dealing with "previews" of PRs.

docs(docs-infra): the build.sh script was renamed to create-image.sh

92c8752

petebacondarwin force-pushed the aio-circleci-builds branch from fba0882 to 92c8752 Compare August 16, 2018 09:28

petebacondarwin merged commit 92c8752 into angular:master Aug 16, 2018

gkalpak mentioned this pull request Aug 23, 2018

ci(docs-infra): how and when to run the PWA-score test #23818

Closed

petebacondarwin deleted the aio-circleci-builds branch August 31, 2018 18:35

gkalpak mentioned this pull request Sep 25, 2018

ci(docs-infra): test PR previews on CI #25671

Closed

3 tasks

gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 26, 2018

refactor(docs-infra): use mockable logger

db0006f

Related discussion: angular#23576 (comment).

gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 26, 2018

docs(docs-infra): update preview server docs to account for recent ch…

b4bbd31

…anges Mostly (but not exclusively) a follow-up to angular#23576.

kara pushed a commit that referenced this pull request Sep 26, 2018

refactor(docs-infra): use mockable logger (#25671)

c7da5d8

Related discussion: #23576 (comment). PR Close #25671

kara pushed a commit that referenced this pull request Sep 26, 2018

docs(docs-infra): update preview server docs to account for recent ch…

9d7ad34

…anges (#25671) Mostly (but not exclusively) a follow-up to #23576. PR Close #25671

kara pushed a commit that referenced this pull request Sep 26, 2018

refactor(docs-infra): use mockable logger (#25671)

fc0a795

Related discussion: #23576 (comment). PR Close #25671

kara pushed a commit that referenced this pull request Sep 26, 2018

docs(docs-infra): update preview server docs to account for recent ch…

d8d276c

…anges (#25671) Mostly (but not exclusively) a follow-up to #23576. PR Close #25671

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Conversation

petebacondarwin commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgorMinar 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!

gkalpak 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!

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!

petebacondarwin commented May 10, 2018

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!

petebacondarwin commented Apr 27, 2018 •

edited

Loading