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

Improvements on routing url option #169

Closed
JakeDawkins opened this issue Jan 12, 2021 · 4 comments · Fixed by #484
Closed

Improvements on routing url option #169

JakeDawkins opened this issue Jan 12, 2021 · 4 comments · Fixed by #484
Assignees
Labels
api 💬 related to registry API changes needs design ✏️ question ❓ help understanding functionality or implementation
Milestone

Comments

@JakeDawkins
Copy link
Contributor

JakeDawkins commented Jan 12, 2021

Related to changes introduced in #130

TODO (jake) -- fill in details here

@abernix abernix added question ❓ help understanding functionality or implementation needs design ✏️ labels Jan 13, 2021
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Feb 5, 2021

What I wrote to @JakeDawkins was that in my ideal world, this is what things would look like.

If --routing-url is not passed and there is already a routing url for a managed service, the API should leave it untouched
If --routing-url is not passed and this is the first time they're pushing a service, we should error and tell them they need to pass the --routing-url flag
If --routing-url is passed and it's the first time pushing a service, it should push the service and exit successfully.
If --routing-url is passed and it's the same routing url as what's currently in the registry, it should warn the user that the routing url will be unchanged, push the service, and exit successfully.
If --routing-url is passed and it's a different routing url than what's currently in the registry, rover should error and require the --force flag to be passed to update the routing URL.


@JakeDawkins had the following to say

If --routing-url is not passed and there is already a routing url for a managed service, the API should leave it untouched

I like this a lot ^^

If --routing-url is not passed and this is the first time they’re pushing a service, we should error and tell them they need to pass the flag

I kinda like this because if they intend on using managed federation, this would be helpful to them

If --routing-url is passed and it’s the first time pushing a service, it should push the service and exit successfully.

If --routing-url is passed and it’s the same routing url as what’s currently in the registry, it should warn the user that the routing url will be unchanged, push the service, and exit successfully.

I don’t see that really being necessary. Especially since it’d probably require a query on our end first to see what the url is, and even then it technically could change between the first query and the mutation to update. Unless of course, we ask for this info in the response. I just don’t think it has a ton of tangible value

If --routing-url is passed and it’s a different routing url than what’s currently in the registry, rover should error and require the --force flag to be passed to update the routing URL.

This also seems unnecessary, since I imagine changing the url is a very common thing rather than a breaking dangerous thing. I think --force makes it feel like “You shouldn’t do this, but we’ll allow it if you’re sure”

@lrlna lrlna added this to the 0.0.4 milestone Feb 19, 2021
@lrlna lrlna added the api 💬 related to registry API changes label Mar 5, 2021
@lrlna lrlna modified the milestones: March 23, April 20 Mar 12, 2021
@lrlna lrlna modified the milestones: April 13, April 27 Apr 8, 2021
@EverlastingBugstopper EverlastingBugstopper removed their assignment Apr 20, 2021
@EverlastingBugstopper
Copy link
Contributor

Re-reading this, I think I stand by my proposal despite @JakeDawkins' comments, with the exception of "emit a warning if --routing-url is passed and it's the same as what's in the registry".

By default, upsert, and require a --force flag to override an old routing url with a new one.

Perhaps the Gravity folks have thought about this a bit more with Launch Control though and we should solicit input from their end.

@JakeDawkins
Copy link
Contributor Author

related to #121

@EverlastingBugstopper EverlastingBugstopper self-assigned this Apr 28, 2021
@EverlastingBugstopper
Copy link
Contributor

ssssoooo, i feel kinda dumb now since the API already handles this pretty much exactly! opening a pr shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 💬 related to registry API changes needs design ✏️ question ❓ help understanding functionality or implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants