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

Feat/forbidden flags #128

Closed
wants to merge 3 commits into from

Conversation

ben-pr-p
Copy link
Contributor

@ben-pr-p ben-pr-p commented Aug 16, 2020

This implements forbidden flags as per #118.

As far as I can tell by running and rerunning the perfTest at various stages of development and at the end, this has no impact on performance for users who choose not to use the feature.

Right now, 14.9.6 is failing with a Postgresql syntax error - do you know if there is something funky with pg going on in that version?

Comment on lines +90 to +92
}

flagsToSkip = await forbiddenFlagsResult;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
flagsToSkip = await forbiddenFlagsResult;
} else {
flagsToSkip = await forbiddenFlagsResult;
}

@benjie
Copy link
Member

benjie commented Aug 17, 2020

Right now, 14.9.6 is failing with a Postgresql syntax error - do you know if there is something funky with pg going on in that version?

Could it be that it's actually PostgreSQL 9.6 kicking out the syntax somewhere? The PR's a little too large for me to quickly scan it and spot the issue.

pg and Node 14 did have weird interactions for a while, but I've only seen it "hanging" rather than giving syntax errors.

I really need to track a version of the PostgreSQL schema in version control so I can track the actual diff.

(I don't have time to fully review this today, but if we can get this and #129 approved I'll merge them into a single migration manually later.)

@benjie
Copy link
Member

benjie commented Aug 21, 2020

I don't have write permission to this branch; please merge my changes from this branch:

https://github.com/graphile/worker/tree/feat/forbidden-flags

(I merged the latest main branch, plus I re-ordered all the forbidden_flags parameters so that they go at the end of the arguments lists so that the changes are non-breaking for people using positional rather than named arguments.)

@benjie
Copy link
Member

benjie commented Aug 21, 2020

I've pushed a huge chunk more work to feat/forbidden-flags

@benjie benjie mentioned this pull request Aug 21, 2020
@benjie
Copy link
Member

benjie commented Aug 21, 2020

The syntax error was indeed a PG9.6 issue; drop function foo; is not supported - you have to supply the argument types.

@benjie
Copy link
Member

benjie commented Aug 21, 2020

(More work on the other branch.)

@ben-pr-p
Copy link
Contributor Author

Closing in favor of #131

@ben-pr-p ben-pr-p closed this Aug 29, 2020
@ben-pr-p ben-pr-p deleted the feat/forbidden-flags branch September 3, 2020 18:09
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.

2 participants