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

Dropped LongviewSubscription from AccountSettingsUpdateOptions #329

Merged
merged 3 commits into from
May 24, 2023

Conversation

ezilber-akamai-zz
Copy link
Contributor

📝 Description

Dropped LongviewSubscription from AccountSettingsUpdateOptions.

@ezilber-akamai-zz ezilber-akamai-zz requested a review from a team May 18, 2023 15:52
@yec-akamai
Copy link
Contributor

Why do we need to remove LongviewSubscription from the update options? It seems longview_subscription is still available in the api doc https://www.linode.com/docs/api/account/#account-settings-update

@ezilber-akamai-zz
Copy link
Contributor Author

Why do we need to remove LongviewSubscription from the update options? It seems longview_subscription is still available in the api doc https://www.linode.com/docs/api/account/#account-settings-update

According to the ticket description LongviewSubscription can no longer be updated through this endpoint.

@yec-akamai
Copy link
Contributor

I see. Is it possible to mark this field deprecated, and add a comment to point user to use PUT /longview/plan? I'm wondering if directly remove it can cause a breaking change.

@lgarber-akamai
Copy link
Contributor

Just to add a bit of context, this field can no longer be sent to the API since the API now returns the following error if longview_subscription is specified:

longview_subscription editing at this location is no longer available. Please use the /longview/plan endpoint instead.

It might still be worth keeping the field to prevent introducing a breaking change for downstream consumers, but it might be worth considering to notify users of the non-functional endpoint. Does anyone have any thoughts on this? 🙂

@ezilber-akamai-zz
Copy link
Contributor Author

Just to add a bit of context, this field can no longer be sent to the API since the API now returns the following error if longview_subscription is specified:

longview_subscription editing at this location is no longer available. Please use the /longview/plan endpoint instead.

It might still be worth keeping the field to prevent introducing a breaking change for downstream consumers, but it might be worth considering to notify users of the non-functional endpoint. Does anyone have any thoughts on this? 🙂

Would a deprecation notice in a comment be enough of a notification for users?

@yec-akamai
Copy link
Contributor

Maybe we can highlight this change in the corresponding release and let the user know the potential breaking change or the API doesn't work this way anymore. I'm also thinking we should let the API doc team know and update the doc as well to reduce confusion.

@ezilber-akamai-zz
Copy link
Contributor Author

I agree that we should probably keep the field to avoid any breaking changes, and I think adding a note to the release is a great idea!

Copy link
Contributor

@lgarber-akamai lgarber-akamai 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!

@ezilber-akamai-zz ezilber-akamai-zz merged commit 858fad0 into linode:main May 24, 2023
3 checks passed
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.

None yet

4 participants