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

Add newmode v2 #1227

Merged
merged 31 commits into from
Jan 27, 2025
Merged

Add newmode v2 #1227

merged 31 commits into from
Jan 27, 2025

Conversation

sharinetmc
Copy link
Contributor

Creates a separate class for NewMode's v2 API since the existing v1 (and the python wrapper the existing NewMode sync is based on will be sunset nest February)

@shaunagm, definitely hoping to discuss Parsons deprecation process with you! Also, wondering if you think v2 should just go in its own folder?
Discussed with @willyraedy some options to go about this. This PR currently follows the 2nd option

  1. so one way to do this is with versions so we publish a new version and people have to upgrade to the version of the package to get the new and only the new NewMode connector
  2. the other way to do it is have latest have both and then do a deprecation process where like:
    NewMode and NewModeV2 are both available and people can update their code
    All users swap to NewModeV2
    Then you switch NewMode to be V2 as well so both NewMode and NewModeV2 are the v2 version
    All users swap back to NewMode
    Delete NewModeV2

@sharinetmc sharinetmc requested a review from mirabuck December 22, 2024 23:14
@shaunagm
Copy link
Collaborator

Will the NewMode v1 API stop working in February?

@shaunagm
Copy link
Collaborator

Rather than creating a new connector with a different name that would have to be edited in lots of different scripts, can we set some kind of variable, either on the connector or at the global config level, that directs which connector to use under the hood?

I'm imagining something like:

class OldNewMode:
   ...

class NewNewMode:
  ...

if os.environ("UPDATE_NEWMODE_VERSION", "False"):
  class NewMode = NewNewMode
else:
  class NewMode = OldNewMode

So then people wouldn't need to change their scripts at all, just use an environmental variable, and then come Feb we just get rid of the OldNewMode entirely. Unless the old version will still be workable for a while...

I'm not 100% sure the above approach will work but it seems like it should?

Copy link
Collaborator

@shaunagm shaunagm left a comment

Choose a reason for hiding this comment

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

Not sure if you wanted me to review the whole thing but I took a quick look, obviously the main thing is the deprecation process - happy to talk about this on a call if you'd prefer

parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/__init__.py Outdated Show resolved Hide resolved
parsons/newmode/newmode_v2.py Outdated Show resolved Hide resolved
parsons/newmode/newmode_v2.py Outdated Show resolved Hide resolved
parsons/newmode/newmode_v2.py Outdated Show resolved Hide resolved
@sharinetmc sharinetmc closed this Jan 5, 2025
@sharinetmc sharinetmc reopened this Jan 5, 2025
@sharinetmc
Copy link
Contributor Author

sharinetmc commented Jan 5, 2025

That sounds like an excellent idea, Shauna! Will make those changes! Also, yes, newmode v1 api will stop working in feburary.

@sharinetmc sharinetmc requested a review from shaunagm January 6, 2025 03:36
@sharinetmc sharinetmc marked this pull request as ready for review January 6, 2025 03:37
@sharinetmc
Copy link
Contributor Author

Marked this as ready for review @shaunagm, since I was able to get that last endpoint working! Happy to discuss versioning stuff further, though (specifically for how you think the best way to share with parsons community will be)!

Copy link
Contributor

@willyraedy willyraedy left a comment

Choose a reason for hiding this comment

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

Great start! Love all the doc strings and there is clearly a lot of detail you've worked through on formatting the requests correctly.

Big things to fix are (1) changing the state in the "get_campaign_ids" method and (2) a consistent return signature from the "converted_request" method. I'm gonna go look at the canales PR now to see how you may be handling the different return values in that script

parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
test/test_newmode/test_newmode.py Show resolved Hide resolved
test/test_newmode/test_newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
@sharinetmc sharinetmc requested a review from willyraedy January 7, 2025 00:36
@mirabuck
Copy link
Collaborator

mirabuck commented Jan 7, 2025

Will the NewMode v1 API stop working in February?

We expect to shut it down completely on February 28th, 2025.

Copy link
Contributor

@willyraedy willyraedy left a comment

Choose a reason for hiding this comment

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

Some last thoughts on how to handle the wonkiness in the Campaigns endpoint but overall this looks good!

parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/newmode/newmode.py Outdated Show resolved Hide resolved
@jburchard
Copy link
Collaborator

I by red d

@sharinetmc
Copy link
Contributor Author

@shaunagm, i think i'm still waiting on your approval for this!

Copy link
Contributor

@IanRFerguson IanRFerguson left a comment

Choose a reason for hiding this comment

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

minimal feedback from me! I think once @shaunagm signs off on this we can ship it

parsons/newmode/newmode.py Outdated Show resolved Hide resolved
parsons/utilities/api_connector.py Show resolved Hide resolved
Copy link
Collaborator

@shaunagm shaunagm left a comment

Choose a reason for hiding this comment

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

Looks good to me, sorry for the delay!

Copy link
Contributor

@IanRFerguson IanRFerguson left a comment

Choose a reason for hiding this comment

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

Nice work!

@sharinetmc sharinetmc merged commit 4b3986e into main Jan 27, 2025
57 checks passed
@sharinetmc sharinetmc deleted the add-newmode-v2 branch January 27, 2025 20:54
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.

6 participants