Skip to content

Make import of SqlBackend fields explicit #3

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

Merged
merged 2 commits into from
May 24, 2021

Conversation

mjgpy3
Copy link
Contributor

@mjgpy3 mjgpy3 commented May 24, 2021

Per this stackage issue: commercialhaskell/stackage#6032

@mjgpy3 mjgpy3 requested review from a team and stackptr and removed request for a team May 24, 2021 14:43
@mjgpy3 mjgpy3 requested review from a team and z0isch and removed request for stackptr and a team May 24, 2021 14:44
Copy link

@z0isch z0isch left a comment

Choose a reason for hiding this comment

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

Hmm- I guess Database.Persist.Sql stopped exporting these fields? I wonder why?

@mjgpy3
Copy link
Contributor Author

mjgpy3 commented May 24, 2021

@z0isch good question! Seems to have happened in this PR: yesodweb/persistent#1225

Maybe this is indicating that we're not building this totally correctly...

@mjgpy3
Copy link
Contributor Author

mjgpy3 commented May 24, 2021

I'll dig in a little more before merging this.

@z0isch
Copy link

z0isch commented May 24, 2021

@z0isch good question! Seems to have happened in this PR: yesodweb/persistent#1225

Maybe this is indicating that we're not building this totally correctly...

Yeah it looks like we should maybe switch to the mkSqlBackend function?

@pbrisbin
Copy link
Member

pbrisbin commented May 24, 2021

Just a headsup that getting green CI doesn't always indicate you've fixed something like this. In most projects, we run CI with the latest LTS resolver. Sometimes we also build with older resolvers. We rarely have a CI job with nightly and we never build with latest Hackage.

That means that if we have a lax upper bound (which we're moving to) we could see breakage with a newer nightly/hackage dependency that won't reproduce on CI.

I'm stilling thinking of the best way to address that fact, for now it's kind of just a "be advised" situation like this. Making a nightly CI job part of our prescribed suites could be good enough (latest Hackage feels like overkill).

@mjgpy3
Copy link
Contributor Author

mjgpy3 commented May 24, 2021

@z0isch I think that that constructor function may not be too great here. Since our pattern to modify an existing backend creating a new one feels tedious and requires me to use those internal fields anyways (to map all existing, required fields that we don't modify over -- looks like 10 fields).

@pbrisbin I tried constraining my local build to take a fresher persistent (persistent >= 2.13.0.0) and this built. Does that serve as an adequate test?

@pbrisbin
Copy link
Member

pbrisbin commented May 24, 2021

@mjgpy3 I'd imagine that works. I would've probably just built with --resolver nightly since that's quicker and should reproduce what the Stackage curators hit.

@mjgpy3 mjgpy3 merged commit 7ce3623 into main May 24, 2021
@pbrisbin pbrisbin deleted the gilli/fix-persistent-imports branch May 24, 2021 18:45
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.

3 participants