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

Fix Postgres to use same value as detected by VCAP_SERVICES #2600

Merged
merged 1 commit into from
Jul 2, 2018
Merged

Fix Postgres to use same value as detected by VCAP_SERVICES #2600

merged 1 commit into from
Jul 2, 2018

Conversation

aeijdenberg
Copy link
Contributor

Description

Fix migration to actually run when Postgres is detected.

ie compare against the value that is set in src/backend/app-core/datastore/datastore.go

Motivation and Context

Prior to this change, we'd see this in the logs:

Discovered Cloud Foundry postgres service and applied config 
No DB Environment detected - skipping migrations

How Has This Been Tested?

Applied change, app now attempts migration (and fails for other reasons, that I'll look into next).

Types of changes

  • [ x ] Bug fix (non-breaking change which fixes an issue)
  • Docs update
  • New feature (non-breaking change which adds functionality)
  • [ ? ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

It's unclear if this is breaking or not. For CF users, it isn't - I'm not sure if the env variable is specifically set by others or not, as I can find no reference to it any any documentation.

Checklist:

  • [ x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have followed the guidelines in CONTRIBUTING.md, including the required formatting of the commit message

@cfdreddbot
Copy link

Hey aeijdenberg!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@codecov
Copy link

codecov bot commented Jul 2, 2018 •
edited
Loading

Codecov Report

Merging #2600 into v2-master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           v2-master    #2600   +/-   ##
==========================================
  Coverage      70.37%   70.37%           
==========================================
  Files            595      595           
  Lines          25202    25202           
  Branches        5689     5689           
==========================================
  Hits           17735    17735           
  Misses          7467     7467

@aeijdenberg
Copy link
Contributor Author

Broken build appears to be unrelated to this change, however I'll rebase off the latest v2-master in case that helps clear the Node version issue in the build.

@aeijdenberg
Copy link
Contributor Author

Travis simply doesn't seem to like Node - pretty sure this is unrelated to this change:

curl: (6) Could not resolve host: nodejs.org

@irfanhabib irfanhabib self-requested a review July 2, 2018 10:05
irfanhabib
irfanhabib approved these changes Jul 2, 2018
View reviewed changes
Copy link
Contributor

@irfanhabib irfanhabib left a comment

Choose a reason for hiding this comment

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

Good catch @aeijdenberg!

@irfanhabib irfanhabib merged commit 569d697 into cloudfoundry:v2-master Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irfanhabib irfanhabib irfanhabib approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aeijdenberg @cfdreddbot @irfanhabib