-
Notifications
You must be signed in to change notification settings - Fork 38
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
update circleci config #4093
update circleci config #4093
Conversation
…docker image to match govcloud DB version.
611fe28
to
c00b0b0
Compare
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.
Looks good Priya! This will make life easier for local CMS development!
c00b0b0
to
971e2f2
Compare
.circleci/config.yml
Outdated
[ -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 |
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.
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)
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.
@rfultz thanks for the suggestion. I will go ahead and remove the npm install webpack
from the script.
.circleci/config.yml
Outdated
@@ -50,8 +51,16 @@ jobs: | |||
pip install -r requirements.txt | |||
|
|||
- run: | |||
name: Install Node.js dependencies | |||
name: Install node dependencies |
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.
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)
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.
@rfultz Its the task name that we see on circleci builds. Do you prefer any other name? Please suggest.
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.
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.
@patphongs the proposed task name sounds good. I will go ahead and update it, unless there is any objection.
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 |
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.
In the build, why did this section end with Build was canceled
?
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.
@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" |
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 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)
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.
@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
@patphongs Like you suggested i update the Node task name to circlebuild: https://app.circleci.com/pipelines/github/fecgov/fec-cms/654/workflows/b48f1c2c-679f-427a-8046-5c6f1d313b2e/jobs/3892 |
Codecov Report
@@ Coverage Diff @@
## develop #4093 +/- ##
===========================================
- Coverage 75.47% 75.45% -0.02%
===========================================
Files 121 121
Lines 7412 7412
Branches 596 596
===========================================
- Hits 5594 5593 -1
- Misses 1818 1819 +1
Continue to review full report at Codecov.
|
@pkfec @patphongs I'm merging this in preparation for innovation sprint as 2 of 3 reviewers have approved. Please let me know if I need to revert. |
Summary (required)
See:
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
Resolves Update circle ci python image to be consistent with runtime version #3978
Include a summary of proposed changes.
Update docker images in circleci/config.yml file :
Screenshots
Before upgrade:
After upgrade:
How to test the changes
locally