[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-605358110 > Each package uses its own set of libraries, including vendoring. Oh I see, you moved the vendor dir down into the `toreqnew`. That makes sense. FWIW I just built and tried this locally too, and I see `atstccfg` requesting 2.0 for `/deliveryservices?cdn=` but 1.4 for everything else. So seems to be working as expected for me at least. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-605344740 It seems like we would have to ~vendor~ copy the old TO client to a new package name in order to differentiate between the vendored and non-vendored client, right? Otherwise the imports will be exactly the same in `toreq` and `toreqnew`, and I guess the vendored client always wins? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-605341313 @rob05c You bring up a good point about the client hiding things from the user -- things it already does today like attempting to re-login once when a 401 is received using `request()`. My idea is similar to that, except it wouldn't have to be a hidden behavior -- it could be optionally enabled when creating a new client instance but disabled by default. We should probably make re-login attempts optional as well. Regardless, I don't think that needs to be done in this PR. Really the entire Go client needs an overhaul. Unmarshalling 1.4 requests into a 1.5 struct without sanitizing to 1.5 defaults does still kind of worry me, but that depends on whether or not the fields actually have defaults to sanitize to. For DSes going from 1.4 to 2.0, they probably don't need sanitized. Anyways, might this be the problem that @mhoppa found? https://github.com/apache/trafficcontrol/pull/4534/files#diff-f0dfb4074818384cd25f1aeb52e7f0f4R42 That looks like it would make the "new" client still use the "old" (vendored) client, right? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-604218435 Maybe I overreacted when I saw this in the PR description: > it serves as an example of the idiom for future dev. To me that came across as "all of our Go components should follow this example in the future". Those kinds of wide-spread decisions that affect all developers in the project need to be communicated so that everyone can get on board. However, based on Neuman's comment, it sounds like this is _not_ supposed to be a decision like that since it's supposed to be a temporary solution just to get ORT to bridge the gap with the 2.0 API. Is that correct @rob05c? Thinking about this more, this seems like an issue that would be permanent -- rather than just bridging the gap from 1.4 to 2.0, we will likely need a solution to bridge the gap from 2.0 to 2.1, 2.1 to 2.2, and so on, indefinitely. For example, let's say ORT is using an unvendored TO Go client that's currently at 2.0. Traffic Ops is currently running 2.0 in Prod. A dev adds a new field for deliveryservices and bumps the version on master to 2.1. The TO Go client on master is then updated to request 2.1. A new release is cut from master, so now the new build of ORT requests 2.1 as well. However, Prod TO is still only running 2.0, and it will 501 any request for 2.1 -- therefore, ORT will not function if it's upgraded before TO is. Using both a vendored and un-vendored version of the Go client _kind of_ solves this problem, but the problem with that is that ORT is _not_ vendoring the corresponding Go structs. So ORT is making 2.0 requests but unmarshalling them into 2.1 structs. Unless ORT also "sanitizes" the 2.1 fields to their default values before using them, this approach will lead to some odd bugs where ORT assumes the wrong default value. If this is going to be the solution going forward, indefinitely, then I think we might have to put some more thought into this approach. If the correct 2.1 defaults are set on the resulting 2.1 struct when a request is made to 2.0 but unmarshalled into a 2.1 struct, I think that would be fine, but sanitizing defaults seems like something that the client itself should handle. In fact, maybe the TO Go client should optionally downgrade the request one minor version if it gets back a 501. For instance `GET /api/2.1/foos` returned a 501, so optionally retry one time with `GET /api/2.0/foos`. If the retry was successful, set the proper 2.1 defaults and return the result. However, it wouldn't try to downgrade across major versions from e.g. 2.0 to 1.4, since we can't always assume that would be safe. Would that work going forward from minor version to minor version, instead of vendoring the TO Go client indefinitely? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-604043645 Yeah, that was not in the unedited version of his comment that I read in my email, and it came off very differently, though I doubt it was intended. All I'm asking is that the community be made aware of paradigm-shifting ideas like this so that other people can be involved before paradigm-shifting PRs get drive-by merged and end up setting the paradigm little input from other project members. Is this not a paradigm shift? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-603997065 > We did, but I guess that was before you came on to the project. We didn't used to do extensive documentation for these things, we just kind of agreed and then did it. Links to the mailing list discussions where decisions have supposedly been made are greatly appreciated if they're going to be used as an excuse for not wanting to open up the discussion to a wider audience. Even if the decision was made 3 years ago, that's a really long time, and a lot of developers who were here 3 years ago are no longer active on the project. We've got a lot of new developers who have no prior knowledge of these discussions, and changing the "TO client paradigm" _today_ (after discussing it 3+ years ago) would be something that would be a great discussion to have _now_, not only as a refresher for devs who were on the project 3+ years ago but also for teaching _newer_ devs. Since decisions like these have ramifications for the entire project, it's important that we not only get it right but also get everyone on board. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-603459860 Ok, I agree that tight coupling is bad, but this seems like something that should be well-communicated with the entire project for everyone to get on board with if our goal is to follow this pattern for every component. This seems more like a backwards-compatibility issue than a coupling issue. API clients will always be coupled to the API, and if a client is coupled to two versions of the same API, doesn't that make it even more tightly coupled to that API? It seems like the trade-off being made is really getting better backwards compatibility with TO at the cost of tighter coupling and increased complexity of components. Maybe we're ok with that, but maybe we're ok with always upgrading TO first? The order doesn't necessarily need to be TO -> TM -> TR -> ORT for example, it could just be "TO first". If we need to fully roll back TO after we've already upgraded most of the other components in the chain, wouldn't we be risking more serious data loss if we rolled back TO at that point rather than hotfixing it and rolling forwards? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops
rawlinp commented on issue #4534: Fix ORT atstccfg to allow using new features in the latest Traffic Ops URL: https://github.com/apache/trafficcontrol/pull/4534#issuecomment-603383227 I'm confused as to why `atstccfg` needs to vendor an old client -- why can't we always stay up to date with master? If we upgrade TO first like we usually do, won't the latest `atstccfg` always be able to use the latest Go client version? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
