-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
refactor: make Junction non-recursive #1818
Conversation
Also Cardinality now includes a Junction or ConstraintName
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 new typing is much better than a revert would have been. And also better than all my suggestions before. This also avoids to have a Junction
with O2M
or something like that. Nice!
I'm not seeing any codecov reports here, yet. Probably because CircleCI was running with your fork when you first uploaded without a PR, @steve-chavez. But didn't you add the codecov token or something like that in your own fork's CircleCI settings to avoid that? I remember we discussed that, at least. Anyway, I will trigger a re-run of the pipeline and then we will see whether how code coverage is with those new types.
Only... that I can't seem to do that. Can't find a way to trigger a build for a PR, that doesn't have one, yet. And I can't rerun the pipeline because it's in your workspace, not postgrest. |
What might have happenend is that Codecov has reverted the token you have set up in your own CircleCI, as a consequence of the security incident. |
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.
Just had a look at the uncovered lines.
cachix push failed: https://app.circleci.com/pipelines/github/PostgREST/postgrest/917/workflows/e3706800-a3aa-4fac-8a55-2573faf1d726/jobs/8771. Hm, circle is missing the cachix auth token right? |
It says:
I remember @monacoremo suggesting to switch from signing key to token or something? In CircleCI there still seems to be signing key, not a token? Do we only need to change the environment variable - or do we need to change the CI script, too? |
Ah, yes, I think so. What puzzled me is that the previous commit was green: https://github.com/PostgREST/postgrest/tree/1f206a560bedf0d7971e9db5e475c37ec311acda.
True, I'll check that out. |
Yeah, that didn't touch any code, so didn't have to upload to cachix. |
We'll need to add a line before we push:
@steve-chavez can you set the env variable up with a separate CI auth token? Happy to fix the CI then |
@monacoremo Done! I set up the Note: the cachix context is already setup at postgrest/.circleci/config.yml Lines 239 to 240 in 698bfe6
|
@steve-chavez perfect, thank you! Just added #1821 |
Also Cardinality now includes a Junction or ConstraintName
While working on #1794, I noticed that we can have recursive Junctions:
postgrest/src/PostgREST/DbStructure/Relation.hs
Lines 43 to 53 in 4b46c4e
Which didn't seem right, as discussed in 5223082#r49568150.
This PR fixes that, also Cardinality now contains the constraint or junction that defines it:
That makes sense IMO, it clears up the code in some parts.