-
Notifications
You must be signed in to change notification settings - Fork 133
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
base: main
Are you sure you want to change the base?
Add newmode v2 #1227
Conversation
@@ -22,6 +22,7 @@ def __init__(self, api_user=None, api_password=None, api_version=None): | |||
Returns: | |||
Newmode class | |||
""" | |||
logger.warning("Newmode V1 API will be sunset in Feburary 2025.") |
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.
definitely need more robust language here, but am hoping to discuss with Shauna along with the Parsons deprecation/versioning process
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.
This sounds pretty good, actually, but once we figure out the approach we might want to provide guidance here.
Will the NewMode v1 API stop working in February? |
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:
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? |
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.
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
@@ -22,6 +22,7 @@ def __init__(self, api_user=None, api_password=None, api_version=None): | |||
Returns: | |||
Newmode class | |||
""" | |||
logger.warning("Newmode V1 API will be sunset in Feburary 2025.") |
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.
This sounds pretty good, actually, but once we figure out the approach we might want to provide guidance here.
@@ -73,6 +73,7 @@ | |||
("parsons.mobilize_america.ma", "MobilizeAmerica"), | |||
("parsons.nation_builder.nation_builder", "NationBuilder"), | |||
("parsons.newmode.newmode", "Newmode"), | |||
("parsons.newmode.newmode_v2", "NewmodeV2"), |
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.
See my comment in the main thread for thoughts on overall design of the deprecation process
return response.status_code | ||
return response | ||
|
||
def converted_request( |
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.
This method needs a docstring, I'll wait for you to add it before asking questions about the method as I imagine some of my questions will be answered by the docstring
`Args:` | ||
campaign_id: str | ||
The ID of the campaign to retrieve. | ||
params: dict |
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.
If this is easy/straightforward to do it would be great to include a link here to newmode's docs to show what params are allowed and how to format them (same for all methods that accept params
)
""" | ||
self.base_url = API_CAMPAIGNS_URL | ||
self.api_version = "jsonapi" | ||
self.headers = { |
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.
It's a little surprising to see headers defined in a get method
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
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