Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Updated GetDeliveryServiceByXMLID function to return a DS nullable struct#4303

Merged
ocket8888 merged 5 commits into
apache:masterfrom
ericholguin:ds-client-get-nullable
Jan 16, 2020
Merged

Updated GetDeliveryServiceByXMLID function to return a DS nullable struct#4303
ocket8888 merged 5 commits into
apache:masterfrom
ericholguin:ds-client-get-nullable

Conversation

@ericholguin

Copy link
Copy Markdown
Contributor

What does this PR (Pull Request) do?

Replaces the struct returned by the GetDeliveryServiceByXMLID client function to DeliveryServiceNullable instead of DeliveryService.

  • This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

  • Traffic Control Go Client

What is the best way to verify this PR?

Run the traffic ops tests

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@ericholguin ericholguin requested a review from ocket8888 January 15, 2020 22:39

@ocket8888 ocket8888 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This breaks CDN-in-a-Box:

./enroller.go:83:16: cannot use dses[0].DeliveryServiceNullableV14.DeliveryServiceNullableV13.DeliveryServiceNullableV12.DeliveryServiceNullableV11.ID (type *int) as type int in return argument
./enroller.go:250:49: cannot use dsID (type *int) as type int in argument to toSession.Session.CreateDeliveryServiceServers

@rob05c

rob05c commented Jan 16, 2020

Copy link
Copy Markdown
Member

Yeah, like @ocket8888 says, this will break existing things, but it also breaks the stability of the Go library, and it will fundamentally break Go Modules if and when we switch to them (which we'll probably have to at some point).

The way we've typically done this in the past is to add a new function with a similar name, e.g. DeliveryServiceByXMLID or GetDeliveryServiceByXMLIDNullable, and add a Deprecated comment to the old one.

@ocket8888

Copy link
Copy Markdown
Contributor

The mailing list decided that TO client methods don't have a deprecation period - but this will need a line in CHANGELOG.md

@ericholguin

ericholguin commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

but this will need a line in CHANGELOG.md

Would this go under Added?

@ocket8888

Copy link
Copy Markdown
Contributor

Uhh sure. If you're doing what rob said and adding a new function it goes under Added. Otherwise I would think under Changed.

@rawlinp

rawlinp commented Jan 16, 2020

Copy link
Copy Markdown
Contributor

Yeah, in addition to the mailing list discussion I'd also add that going the deprecation route would be great if we actually cleaned up deprecated functions, stopped increasing the usage of already-deprecated functions, etc. But historically nobody has really gone back to clean things up, and I've definitely encountered new code that used already-deprecated client methods. At least if you make a breaking change to the TO Go client you're forced to fix all the existing usages in the repo (otherwise the build fails), and we don't end up w/ 3 different client methods that all do the same thing.

@ocket8888 ocket8888 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, all unit and api/client integration tests are passing, all components build and function normally.

@ocket8888 ocket8888 merged commit d943559 into apache:master Jan 16, 2020
@ericholguin ericholguin deleted the ds-client-get-nullable branch May 10, 2022 19:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants