-
Notifications
You must be signed in to change notification settings - Fork 11.9k
docs: update the continuous integration story #11492
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, thanks for updating this!
I left a question about npm ci
, a naming nitpick, and since you mentioned #11116 (comment) I would like to ask you to add it to.
Using ChromeHeadless has some caveats to it (see https://github.com/angular/protractor/blob/master/docs/browser-setup.md#using-headless-chrome) but I think for most users it should be ok so no need to highlight it.
I also feel you've succeeded in maintaining the style of the original doc, cheers!
|
||
# install the latest npm to allow use of 'npm ci' | ||
- run: sudo npm install -g npm@latest | ||
- run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if npm ci
is faster than using cache on the CI? Last time I checked npm ci
was much slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're quite right, using the cache is much faster.
ChromeNoSandbox: { | ||
base: 'Chrome', | ||
flags: ['--no-sandbox'] | ||
myChromeHeadless: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: what do you think of ChromeHeadlessCI
instead? I feel the my
prefix doesn't say much about what's going on.
@@ -76,28 +79,23 @@ jobs: | |||
build: | |||
working_directory: ~/my-project | |||
docker: | |||
- image: circleci/node:8-browsers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you provide only the major version, docker will always choose the most recent version.
E.g. image: circleci/node:10-browsers
and then running node -v
yields: v10.6.0
I would recommend reverting this change back to - image: circleci/node:8-browsers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @markgoho, good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look good, but due to the way we parse commit messages you will need to squash all of these into a single commit.
I think the easiest way to do this is rebasing and marking the second and third commits as fixups.
adef286
to
0fe94cb
Compare
update the CircleCi and Travis configurations update ng test and ng e2e command flags for version 6 use headless chrome instead of chrome add guidance on ChromeDriver closes angular#10677
0fe94cb
to
db1d410
Compare
Thanks @filipesilva ! I've rebased, squashed the commits into a single commit with an updated message (to remove the reference to npm ci and add a reference to the guidance on ChromeDriver). This is my first time contributing and I'm not totally confident I have the git part the way you would like, so please forgive me if I've made a mistake here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rebase went well, cheers!
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This PR aims to update the CircleCi and Travis configurations in the continuous integration story.
The main changes are:
ng test
andng e2e
command flags for version 6npm ci
instead ofnpm install
To assist review:
I have endeavored to maintain the style and simplicity of the story. All comments are very welcome.
@filipesilva, I noticed this comment and would be happy to include what you said in this PR.
I feel this PR closes #10677 because the essential problem there was that the flags for the
ng test
andng e2e
commands changed with version 6. This PR also builds on #11445 and updates theng test
--browsers flag
and theng e2e
--protractor-config
flag, bringing the flags up to date with version 6.