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

(refactor): Update circleci config to circleci workflowsv2 #3618

Closed
wants to merge 16 commits into from

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Jan 31, 2018

Refactor circleci config.

Part of #2858.

This PR updates:

  1. the circleci config.yml to use workflows which enables:
    • faster builds running jobs within a workflow independently
    • faster re-runs by allowing re-runs on each job of a workflow
  2. updates the test node image to a -browsers image that contains headless Chrome available for running client tests

This PR adds test_publish_deploy workflow with three jobs (which we already had before):

  • tests: which runs the app tests
  • publish - builds docker image and publish
  • deployDocs - runs jsdoc and deploys to aws


docker:
- image: node:8
- image: circleci/node:8.9.3-browsers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this per comment made here

paths:
- ~/.meteor
# install OS dependencies
- run: sudo apt-get update
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added sudo here (and a couple other places) because the new node image throws permission denied errors on those commands. If there's a better way to this please point me to it...

key: dev_bundle-{{ .Branch }}

# install Reaction CLI
- run: sudo npm install -g reaction-cli
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done previously with yarn. I switched to npm here (for consistency), but I'll like to know if there's reason why yarn is preferred.

working_directory: /home/reaction
# run automated tests
tests:
working_directory: ~/reaction
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed from /home/reaction to ~/reaction. The 8.9.3-browsers image throws permission denied error on root folder. Any better way?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @impactmass. This is a minor suggestion because I'm sure that this will work as-is. ~ is expanded to home by the shell when it's not prefixed. But ~ is a typing convenience for the command line. In scripts it's generally preferred to use the $HOME var.

See:


That said, you did the right thing. The old path didn't work on that image because the user reaction doesn't exist in circleci/node:8.9.3-browsers and therefore the /home/reaction directory doesn't exist either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I changed to $HOME. With that circleci fails on the checkout step.
Unable to create directory '$HOME/reaction. Not sure why yet.

Also, I did not get this part The old path didn't work on that image because the user reaction doesn't exist ... and therefore the /home/reaction. Were we creating a reaction user in the previous setup?

Copy link
Contributor

@jshimko jshimko Jan 31, 2018

Choose a reason for hiding this comment

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

There was never a reaction user. The previous container ran as root. That was just the directory I picked to checkout the code within the image. And ~/ definitely was a problem previously, so that's why I explicitly set it to /home/.

- restore_cache:
name: Restoring Meteor cache
key: meteor
key: meteor-{{ .Branch }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added branch name to the keys for the cache primarily because the old cache that was generated on previous node:8 image is to be linked/restored to root locations on the new 8.9.3 (which also resulted in permission denied errors).


- save_cache:
paths:
- ~/node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not where the app's node_modules are. It's ~/reaction/node_modules

@impactmass impactmass changed the title [WIP] refactor Update circleci config to circleci workflowsv2 (refactor): Update circleci config to circleci workflowsv2 Feb 9, 2018
@impactmass
Copy link
Contributor Author

@jshimko please do a full review of the changes to config.yml and install.sh (the other commits are from a PR #3629 which is already in release-1.8.0). If this is all good, it can go with release-1.8.0, otherwise I will attend to requested changes when I'm back.

@brent-hoover
Copy link
Collaborator

@impactmass Do you still need @jshimko to look at this or are you working with @ticean on it?

@impactmass impactmass changed the title (refactor): Update circleci config to circleci workflowsv2 [WIP] (refactor): Update circleci config to circleci workflowsv2 Feb 28, 2018
@impactmass impactmass changed the title [WIP] (refactor): Update circleci config to circleci workflowsv2 (refactor): Update circleci config to circleci workflowsv2 Feb 28, 2018
@impactmass impactmass requested review from ticean and removed request for jshimko February 28, 2018 00:24
@impactmass
Copy link
Contributor Author

closing this, will be opened as part of the linked issue above

@impactmass impactmass closed this Mar 1, 2018
@impactmass impactmass deleted the impactmass-feat-circle-workflows branch March 7, 2018 22:24
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