FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix custom PostgreSQL domains not being used in table definitions by lw · Pull Request #1073 · cms-dev/cms · GitHub

/ cms Public

Fix custom PostgreSQL domains not being used in table definitions - #1073

Merged
lw merged 3 commits into
cms-dev:masterfrom
lw:fix_domains
Dec 7, 2018
ConversationCommits (3)ChecksFiles changed
Merged

Fix custom PostgreSQL domains not being used in table definitions#1073
lw merged 3 commits into
cms-dev:masterfrom
lw:fix_domains

Conversation

lw commented Nov 11, 2018 •
edited by stefano-maggiolo
Loading

Copy link
Copy Markdown
Member

Spotted thanks to #1071.

Needs to be backported to v1.4.

This change is 

lw requested a review from stefano-maggiolo November 11, 2018 21:04
lw mentioned this pull request Nov 11, 2018
Closed

codecov Bot commented Nov 11, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1073 into master will increase coverage by 0.05%.
The diff coverage is 77.92%.

@@            Coverage Diff            @@
##           master   #1073      +/-   ##
=========================================
+ Coverage   62.05%   62.1%   +0.05%     
=========================================
  Files         228     229       +1     
  Lines       16457   16521      +64     
=========================================
+ Hits        10212   10260      +48     
- Misses       6245    6261      +16
Flag Coverage Δ
#functionaltests 45.8% <9.09%> (-0.25%) ⬇️
#unittests 43.27% <77.92%> (+0.13%) ⬆️
Impacted Files Coverage Δ
cms/db/__init__.py 96.42% <100%> (ø) ⬆️
cms/db/types.py 100% <100%> (+7.04%) ⬆️
cmscontrib/updaters/update_41.py 73.84% <73.84%> (ø)
cms/server/contest/handlers/taskusertest.py 41.21% <0%> (-1.82%) ⬇️
cms/service/ScoringService.py 66.15% <0%> (-1.54%) ⬇️
cms/service/esoperations.py 78.01% <0%> (-1.42%) ⬇️
cms/server/admin/handlers/dataset.py 24.76% <0%> (-0.93%) ⬇️
cms/db/filecacher.py 77.7% <0%> (-0.66%) ⬇️
cms/grading/Job.py 88.62% <0%> (-0.48%) ⬇️
cms/db/util.py 51.85% <0%> (-0.43%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e0910b...89d0166. Read the comment docs.

stefano-maggiolo requested changes Nov 11, 2018
View reviewed changes

stefano-maggiolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we really need to backport?

Also, shouldn't we bump db version and copy the updater?

Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

lw commented Nov 11, 2018

Copy link
Copy Markdown
Member Author

The reason for backporting is that we just got a bug report due to this.

I added a no-op updater that just prints a message and fails if any data is invalid. Is this what you had in mind?

stefano-maggiolo requested changes Nov 12, 2018
View reviewed changes

stefano-maggiolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Uhm, I was thinking at copying update_26.py, but a failure is fine too.

I'm not too keen in backporting something that requires a db update when it's not fixing a too important bug, and that bug had been present for a long time. Opinions?

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @codacy-bot and @lerks)

isolate, line 1 at r2 (raw file):

Subproject commit 5d6beb258e995cf4b0c982ff98cbbc0e9e127853

Intended?

cmscontrib/updaters/update_41.py, line 24 at r2 (raw file):

This updater makes sure filenames contain no percent sign, except in a
trailing ".%l", if any.

Wrong comment? Possibly mention that this is a duplicare and the reason.

stefano-maggiolo requested changes Nov 12, 2018
View reviewed changes

stefano-maggiolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @codacy-bot and @lerks)

cmscontrib/updaters/update_41.py, line 20 at r2 (raw file):

"""A class to update a dump created by CMS.

Would be nice to print what is wrong - the idea is that people might have created bad names between db 26 and now, and so they might legitimately have bad names not for their fault.

lw commented Nov 27, 2018
View reviewed changes

lw left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can avoid backporting now, and do it if this bug starts biting too many people.

Dismissed @codacy-bot from a discussion.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @stefano-maggiolo)

cmscontrib/updaters/update_41.py, line 20 at r2 (raw file):

Previously, stefano-maggiolo (Stefano Maggiolo) wrote…

Would be nice to print what is wrong - the idea is that people might have created bad names between db 26 and now, and so they might legitimately have bad names not for their fault.

Done.

cmscontrib/updaters/update_41.py, line 24 at r2 (raw file):

Previously, stefano-maggiolo (Stefano Maggiolo) wrote…

Wrong comment? Possibly mention that this is a duplicare and the reason.

Done.

isolate, line 1 at r2 (raw file):

Previously, stefano-maggiolo (Stefano Maggiolo) wrote…

Intended?

No, dunno how that happened, reverted

cms-dev deleted a comment Nov 27, 2018
stefano-maggiolo reviewed Dec 2, 2018
View reviewed changes

stefano-maggiolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @stefano-maggiolo and @codacy-bot)

stefano-maggiolo approved these changes Dec 2, 2018
View reviewed changes

stefano-maggiolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codacy-bot)

lw commented Dec 7, 2018
View reviewed changes

lw left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Dismissed @codacy-bot from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

lw merged commit 9da2d14 into cms-dev:master Dec 7, 2018
lw deleted the fix_domains branch December 7, 2018 21:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

stefano-maggiolo stefano-maggiolo approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL