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

chore: eject pg-compose #1403

Merged
merged 8 commits into from
Sep 20, 2022
Merged

chore: eject pg-compose #1403

merged 8 commits into from
Sep 20, 2022

Conversation

bchrobot
Copy link
Member

@bchrobot bchrobot commented Sep 4, 2022

Description

Ejects pg-compose library.

Motivation and Context

  • Less complexity
  • Fewer things to maintain
  • Consistent and up to date API and typedefs across libs

Specifically in preparation for tweaking application entry points for serverless.

How Has This Been Tested?

This has (not yet) been tested locally:

  • Migrating graphile-worker and graphile-scheduler
  • Scheduled tasks
  • Fetching activist codes, result codes, survey questions, and saved lists
  • Importing a saved list to a campaign

Screenshots (if appropriate):

N/A

Documentation Changes

N/A

Checklist:

  • My change requires a change to the documentation.
  • I have included updates for the documentation accordingly.

@bchrobot bchrobot added this to the 4.38.0 milestone Sep 9, 2022
@bchrobot bchrobot force-pushed the chore-eject-pg-compose branch from 87557a3 to 532d7b6 Compare September 9, 2022 16:09
@bchrobot bchrobot marked this pull request as ready for review September 9, 2022 16:33
Copy link
Contributor

@hiemanshu hiemanshu left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few suggestions.

@@ -750,6 +750,9 @@ const validators = {
"The numeric coding of the VAN list export type. The default is the Hustle format.",
default: 8
}),
EXPORT_JOB_WEBHOOK: url({
default: "https://eneeuk8v5vhvsc8.m.pipedream.net"
Copy link
Contributor

@hiemanshu hiemanshu Sep 13, 2022

Choose a reason for hiding this comment

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

Question: What URL is this? Is it something okay being in a public repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

No issue with it being in a public repo. It's unclear if it's a required option for the export request so it just points at a webhook catcher (that is turned off currently).

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Sounds good!

@@ -24,26 +27,29 @@ export interface VANActivistCode {

export const fetchVANActivistCodes: Task = async (
payload: GetActivistCodesPayload,
_helpers: any
helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type JobHelpers

Copy link
Member Author

Choose a reason for hiding this comment

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

The underscore is for the no-unused-vars rule:

Spoke/.eslintrc.js

Lines 77 to 80 in 26da320

"@typescript-eslint/no-unused-vars": [
"error",
{ argsIgnorePattern: "^_", varsIgnorePattern: "^_" }
],

I'll remove the any typing though -- it should be able to infer the JobHelpers type from Task but I'll set that if not

Copy link
Contributor

Choose a reason for hiding this comment

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

The _ and any part is being removed, and helpers is now being used in there. My suggestion was to only add the type, helps IDEs. I think my suggestion was confusing, sorry about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just went through on the latest commit and confirmed that for each of these, the JobHelpers type is inferred correctly.

Each of these is now using helpers for fetching VAN authentication so the leading _s have been removed as well.

@@ -17,21 +20,25 @@ export interface VANResultCode {

export const fetchVANResultCodes: Task = async (
payload: GetResultCodesPayload,
_helpers: any
helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type JobHelpers

@@ -32,8 +35,10 @@ export interface VANSurveyQuestion {

export const fetchVANSurveyQuestions: Task = async (
payload: GetSurveyQuestionsPayload,
_helpers: any
helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type JobHelpers


export const getSavedLists: Task = async (
payload: GetSavedListsPayload,
helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Add type JobHelpers

@bchrobot bchrobot merged commit 088ece9 into main Sep 20, 2022
@bchrobot bchrobot deleted the chore-eject-pg-compose branch September 20, 2022 15:21
@bchrobot bchrobot mentioned this pull request Oct 10, 2022
2 tasks
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