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

Enscoresw 3267 #435

Merged
merged 2 commits into from
Nov 5, 2019
Merged

Enscoresw 3267 #435

merged 2 commits into from
Nov 5, 2019

Conversation

ameya1981
Copy link

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion;
  • Review the contributing guidelines for this repository; remember in particular:
    • do not modify code without testing for regression
    • provide simple unit tests to test the changes
    • if you change the schema you must patch the test databases as well, see Updating the schema
    • the PR must not fail unit testing

Description

Make 'type' column in core.external_db table to NOT NULL

Use case

As requested in ENSINT-307
Healtchecks were checking for Not Null, but table was allowing null values.

Benefits

The health-check can be deprecated

Possible Drawbacks

Value needs to be entered even if not applicable

Testing

No unit tests changed

If so, do the tests pass/fail?
N/A

Have you run the entire test suite and no regression was detected?
No regression detected

@mkszuba mkszuba changed the base branch from release/98 to master November 1, 2019 12:55
@mkszuba mkszuba requested a review from tgrego November 1, 2019 13:25
Copy link
Contributor

@mkszuba mkszuba left a comment

Choose a reason for hiding this comment

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

In addition to the Variation schema not being up to date, I am afraid the description of your second commit is not accurate - it changes more than table.sql and applies more than this one patch. Could you change it to something along the lines of "patched test databases"?

@coveralls
Copy link

coveralls commented Nov 1, 2019

Coverage Status

Coverage decreased (-0.02%) to 81.459% when pulling a8a76c4 on ENSCORESW-3267 into 940e1fb on master.

Copy link
Contributor

@mkszuba mkszuba left a comment

Choose a reason for hiding this comment

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

I am afraid the description of your second commit is not accurate - it changes more than table.sql and applies more than this one patch. Could you change it to something along the lines of "patched test databases"?

Includes recent Variation schema updates.
Copy link
Contributor

@mkszuba mkszuba left a comment

Choose a reason for hiding this comment

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

Alas, that last change has simply been a new empty commit instead of an amendment to the previous one. I'll just squash these into one for you this time.

@mkszuba mkszuba merged commit a244e1e into master Nov 5, 2019
@mkszuba mkszuba deleted the ENSCORESW-3267 branch November 5, 2019 15:22
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