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

Refactor: interface selections have proper location #107

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

gmac
Copy link
Contributor

@gmac gmac commented Nov 12, 2021

As described in #113, it's less than ideal that namespaces and interfaces are omitted from the Locations map, and therefore get handled as error side-effects:

loc, err := ctx.Locations.URLFor(parentType, location, selection.Name)
// Errors are returned for unmapped namespace/interface locations
if err == nil && loc != location {
  // ...
}

These missing locations bank on a not-obvious rule that they're not allowed to transition services, so can alway use their parent object's location. Ideally these known resources would simply route to known locations, and missing location errors would be treated as actual errors.

This adds interfaces to the Locations map so they become a properly routed resource.

@gmac gmac requested a review from a team as a code owner November 12, 2021 02:09
@gmac gmac changed the title Fix: interface selections have no location Optional fix? Interface selections have no location Nov 12, 2021
@lucianjon
Copy link
Contributor

Sorry this one took a while to get to @gmac, will take a look over the next day or so if someone doesn't get to it first

@gmac gmac force-pushed the fix-interface-locations branch from 407a412 to e74caaa Compare November 16, 2021 03:06
@gmac gmac changed the title Optional fix? Interface selections have no location Refactor: interface selections have proper location Nov 16, 2021
@lucianjon
Copy link
Contributor

Thanks @gmac, even if it's optional at this point I think this is still definitely worthwhile. I actually have a hunch this will work towards fixing a bug with some internal tooling that relies on the meta plugin. I'll have a play with it after merging this in but I assume I'll need to add interface to the case statement here too.

@lucianjon lucianjon merged commit 17c7b97 into movio:main Nov 17, 2021
@gmac gmac deleted the fix-interface-locations branch November 17, 2021 04:14
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.

2 participants