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.

Sign up for GitHub

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

Jump to bottom

Drop dependencies for cols needs a CASCADE #819

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Drop dependencies for cols needs a CASCADE #819

merged 4 commits into from
Jun 6, 2023

Conversation

elyse-weiss
Copy link
Contributor

Closes #818

Jason94 and others added 2 commits May 8, 2023 17:04
@Jason94 @austinweisgrau
@IanRFerguson @codygordon @sjwmoveon @technicalex @crayolakat @willyraedy
Merge main into major-release (#814)
ee4ec8b 
* Use black formatting in addition to flake8 (#796)

* Run black formatter on entire repository

* Update requirements.txt and CONTRIBUTING.md to reflect black format

* Use black linting in circleci test job

* Use longer variable name to resolve flake8 E741

* Move noqa comments back to proper lines after black reformat

* Standardize S3 Prefix Conventions (#803)

This PR catches exception errors when a user does not exhaustive access to keys in an S3 bucket

* Add Default Parameter Flexibility (#807)

Skips over new `/` logic checks if prefix is `None` (which is true by default)

* MoveOn Shopify / AK changes (#801)

* Add access_token authentication option for Shopify

* Remove unnecessary check
The access token will either be None or explicitly set; don't worry about an empty string.

* Add get_orders function and test

* Add get_transactions function and test

* Add function and test to get order

* style fixes

* style fixes

---------

Co-authored-by: sjwmoveon <[email protected]>
Co-authored-by: Alex French <[email protected]>
Co-authored-by: Kathy Nguyen <[email protected]>

* Catch File Extensions in S3 Prefix (#809)

* add exception handling

* Shortened logs for flake8

* add logic for default case

* added file logic + note to user

* restructured prefix logic

This change moves the prefix -> prefix/ logic into a try/except block ... this will be more robust to most use cases, while adding flexibility that we desire for split-permission buckets

* drop nested try/catch + add verbose error log

* Add error message verbosity

Co-authored-by: willyraedy <[email protected]>

---------

Co-authored-by: willyraedy <[email protected]>

---------

Co-authored-by: Austin Weisgrau <[email protected]>
Co-authored-by: Ian <[email protected]>
Co-authored-by: Cody Gordon <[email protected]>
Co-authored-by: sjwmoveon <[email protected]>
Co-authored-by: Alex French <[email protected]>
Co-authored-by: Kathy Nguyen <[email protected]>
Co-authored-by: willyraedy <[email protected]>
cascade
0102fac
@shaunagm
Copy link
Collaborator

Looks like there's an issue with linting. You can now run a single black command to fix linting issues. Unfortunately the docs are not up-to-date with our process because we haven't cut a release lately, but you can see the commands in the most recent docs:

flake8 --max-line-length=100 --extend-ignore=E203,W503 parsons
black parsons

@shaunagm shaunagm mentioned this pull request May 19, 2023
Open
@elyse-weiss
Copy link
Contributor Author

@Jason94 let me know how I can point this toward the major release

@elyse-weiss elyse-weiss changed the base branch from main to major-release May 23, 2023 15:15
@elyse-weiss
Copy link
Contributor Author

@Jason94 can you confirm I didn't mess anything up when I merged conflicts?

@shaunagm
Copy link
Collaborator

shaunagm commented Jun 5, 2023

Let's get this landed. I think per our discussion on Slack, this doesn't actually need to target the major release. @elyse-weiss, can you confirm that the method you're altering, drop_dependencies_for_cols, is not exposed in the documentation, and that the methods that do call it say that the drop cascades, even though it currently doesn't?

@elyse-weiss
Copy link
Contributor Author

Yes, this change is in line with current documentation.

@shaunagm
Copy link
Collaborator

shaunagm commented Jun 6, 2023 •
edited
Loading

Okay, great! Can you update this to point back to main instead of major release, then? Sorry for the back-and-forth and accompanying delay.

@elyse-weiss elyse-weiss changed the base branch from major-release to main June 6, 2023 14:44
@elyse-weiss
Copy link
Contributor Author

done! should I go ahead and merge?

@shaunagm
Copy link
Collaborator

shaunagm commented Jun 6, 2023

Yup, we're good to go!

@elyse-weiss elyse-weiss merged commit 78d7f86 into main Jun 6, 2023
@elyse-weiss elyse-weiss deleted the elyse-depedency-cascade branch June 6, 2023 14:55
elyse-weiss pushed a commit that referenced this pull request Jun 6, 2023
Revert "Drop dependencies for cols needs a CASCADE (#819)"
64241f8 
This reverts commit 78d7f86.
@elyse-weiss elyse-weiss mentioned this pull request Jun 6, 2023
Merged
shaunagm pushed a commit that referenced this pull request Jun 6, 2023
Revert "Drop dependencies for cols needs a CASCADE (#819)" (#832)
52ec230 
This reverts commit 78d7f86.
@shaunagm shaunagm mentioned this pull request Jun 6, 2023
Merged
talevy42 pushed a commit to talevy42/parsons that referenced this pull request Jul 8, 2023
@Jason94 @austinweisgrau
@IanRFerguson @codygordon @sjwmoveon @technicalex @crayolakat @willyraedy @tal42levy
Drop dependencies for cols needs a CASCADE (move-coop#819)
7386621 
* Merge main into major-release (move-coop#814)

* Use black formatting in addition to flake8 (move-coop#796)

* Run black formatter on entire repository

* Update requirements.txt and CONTRIBUTING.md to reflect black format

* Use black linting in circleci test job

* Use longer variable name to resolve flake8 E741

* Move noqa comments back to proper lines after black reformat

* Standardize S3 Prefix Conventions (move-coop#803)

This PR catches exception errors when a user does not exhaustive access to keys in an S3 bucket

* Add Default Parameter Flexibility (move-coop#807)

Skips over new `/` logic checks if prefix is `None` (which is true by default)

* MoveOn Shopify / AK changes (move-coop#801)

* Add access_token authentication option for Shopify

* Remove unnecessary check
The access token will either be None or explicitly set; don't worry about an empty string.

* Add get_orders function and test

* Add get_transactions function and test

* Add function and test to get order

* style fixes

* style fixes

---------

Co-authored-by: sjwmoveon <[email protected]>
Co-authored-by: Alex French <[email protected]>
Co-authored-by: Kathy Nguyen <[email protected]>

* Catch File Extensions in S3 Prefix (move-coop#809)

* add exception handling

* Shortened logs for flake8

* add logic for default case

* added file logic + note to user

* restructured prefix logic

This change moves the prefix -> prefix/ logic into a try/except block ... this will be more robust to most use cases, while adding flexibility that we desire for split-permission buckets

* drop nested try/catch + add verbose error log

* Add error message verbosity

Co-authored-by: willyraedy <[email protected]>

---------

Co-authored-by: willyraedy <[email protected]>

---------

Co-authored-by: Austin Weisgrau <[email protected]>
Co-authored-by: Ian <[email protected]>
Co-authored-by: Cody Gordon <[email protected]>
Co-authored-by: sjwmoveon <[email protected]>
Co-authored-by: Alex French <[email protected]>
Co-authored-by: Kathy Nguyen <[email protected]>
Co-authored-by: willyraedy <[email protected]>

* cascade

* black

---------

Co-authored-by: Jason <[email protected]>
Co-authored-by: Austin Weisgrau <[email protected]>
Co-authored-by: Ian <[email protected]>
Co-authored-by: Cody Gordon <[email protected]>
Co-authored-by: sjwmoveon <[email protected]>
Co-authored-by: Alex French <[email protected]>
Co-authored-by: Kathy Nguyen <[email protected]>
Co-authored-by: willyraedy <[email protected]>
Co-authored-by: Elyse Weiss <[email protected]>
talevy42 pushed a commit to talevy42/parsons that referenced this pull request Jul 8, 2023
@tal42levy
Revert "Drop dependencies for cols needs a CASCADE (move-coop#819)" (m…
2d82dde 
…ove-coop#832)

This reverts commit 78d7f86.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jason94 Jason94 Jason94 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Drop dependencies for cols needs a CASCADE
3 participants
@elyse-weiss @shaunagm @Jason94