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

update circleci config #4093

Merged
merged 5 commits into from
Oct 11, 2020
Merged

update circleci config #4093

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42ec239
Update python docker image to match the runtime version and postgres …
pkfec Sep 30, 2020
026043d
Add script in cirleci config to account for node upgrades.
pkfec Sep 30, 2020
971e2f2
Update postgres docker image to match the cloud.gov cms db version
pkfec Oct 1, 2020
bf9b127
Delete npm install webpack from the script. npm install will install …
pkfec Oct 6, 2020
f3e2a56
Update the node task name to be more specific
pkfec Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,16 @@ jobs:
# use `-browsers` prefix for selenium tests, e.g. `<image_name>-browsers`

# Python
- image: circleci/python:3.7.4-stretch-node-browsers
- image: circleci/python:3.7.9-stretch-node-browsers
environment:
TZ: America/New_York
DATABASE_URL: postgres://postgres@0.0.0.0/cfdm_cms_test

# PostgreSQL
- image: circleci/postgres:9.6.8
- image: circleci/postgres:11.9
Copy link
Contributor

Choose a reason for hiding this comment

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

In the build, why did this section end with Build was canceled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfultz - Its a circleci bug that is around for a while.
See here : https://discuss.circleci.com/t/container-step-shows-build-was-cancelled-and-is-a-missing-the-green-checkmark-although-all-tests-run-and-pass/37600

environment:
POSTGRES_USER: postgres
POSTGRES_HOST_AUTH_METHOD: "trust"
Copy link
Contributor

Choose a reason for hiding this comment

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

The build is giving us a warning about this line (POSTGRES_HOST_AUTH_METHOD). Is this something we want to do outside of local? (The warning looks important but I'd rely 100% on others' expertise)

Copy link
Contributor Author

@pkfec pkfec Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@rfultz - We had those warning prior to the postgres docker upgrade. With older docker version, the circleci would silently, ignore the warning and let the build run.
Unless the ENV variable is specified, the builds will fail.

I mentioned in my PR already on why the ENV variable needs to be set and posting here again for your reference:https://discuss.circleci.com/t/postgresql-image-password-not-specified-issue/34555?utm_medium=SEM&utm_source=gnb&utm_campaign=SEM-gb-DSA-Eng-uscan&utm_content=&utm_term=dynamicSearch-&gclid=CjwKCAjw_NX7BRA1EiwA2dpg0hsoFeu7z02id7KLIrJiImZpUfDShDzw7DbAwJJdL75bstnqP4914RoCoWAQAvD_BwE

POSTGRES_DB: cfdm_cms_test

working_directory: ~/repo
Expand All @@ -31,7 +32,7 @@ jobs:
# https://circleci.com/docs/2.0/postgres-config/#postgresql-circleci-configuration-example
command: |
sudo apt-get update -qq && sudo apt-get install -y build-essential postgresql-client
echo 'export PATH=/usr/lib/postgresql/9.6/bin/:$PATH' >> $BASH_ENV
echo 'export PATH=/usr/lib/postgresql/10.11/bin/:$PATH' >> $BASH_ENV
echo "en_US.UTF-8 UTF-8" | sudo tee /etc/locale.gen
sudo locale-gen en_US.UTF-8

Expand All @@ -50,8 +51,16 @@ jobs:
pip install -r requirements.txt

- run:
name: Install Node.js dependencies
name: Install node dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Nowhere near a dealbreaker but "Install Node dependencies" doesn't really describe what's happening here. The node packages are being installed, yes, but it's also building the entire front-end of the site (JavaScript, CSS/scss, image copying)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfultz Its the task name that we see on circleci builds. Do you prefer any other name? Please suggest.

Copy link
Member

Choose a reason for hiding this comment

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

@rfultz @pkfec What about something like this? "Install node and build web assets". Seems like a generic way of saying we're building everything from JS/scss/images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patphongs the proposed task name sounds good. I will go ahead and update it, unless there is any objection.

command: |
curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.6/install.sh | bash
echo ". ~/.nvm/nvm.sh" >> $BASH_ENV
export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion" # This loads nvm bash_completion
nvm install 10.16.0
nvm use 10.16.0
nvm alias default 10.16.0
sudo npm install -g npm
npm install webpack
Copy link
Contributor

Choose a reason for hiding this comment

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

Outside the scope of this ticket, but we don't need npm install webpack if we also have npm install (and it's on the next line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfultz thanks for the suggestion. I will go ahead and remove the npm install webpack from the script.

npm install
Expand Down