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

Tolerate component routes not starting with '/' #1500

Merged
merged 1 commit into from
May 17, 2023

Conversation

itowlson
Copy link
Contributor

Fixes #1493.

An alternative solution would be to fail validation if any component route didn't start with '/'. That would be safer in the sense of not committing us to supporting "wrong" manifests, but would risk a user experience of "stupid computer, you know what's wrong so why do you need me to fix it." Happy to switch to that if people prefer.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

LGTM. I think I prefer the magic correction here as opposed to a validation error but I could lean the other way if others feel strongly.

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I think I'm ok with us not requiring the user explicitly put a "/" before the route, but there are a few more cases that I think we should think about:

  • "..."
  • ""

@fibonacci1729
Copy link
Contributor

@rylev I would assume that those 2 cases, given this PR would become "/..." and "/" respectively; these patterns are handled already.

@itowlson
Copy link
Contributor Author

@fibonacci1729 I think you are right, but maybe those should be rejected: the specification is that wildcards end in /..., and the empty string is more likely to be a mistake than an intentional choice.

(I fear this is the risk of extending tolerance instead of rejecting... we have to figure out how/whether to tolerate the edge cases too, and what consistent behaviour looks like in those situations!)

@rylev Given time zone differences it would be super useful if you could share your preferred behaviour and thoughts on acceptable trade-offs - if we have to iterate on it interactively, the PR is going to be very drawn out, and I don't think these cases are worth keeping on my stack for that long if you see what I mean?

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I think we should merge this as is. We can always improve in the future if need be.

@itowlson
Copy link
Contributor Author

@rylev awesome, thanks!

@itowlson itowlson merged commit 2c776aa into fermyon:main May 17, 2023
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.

Forgotten / before route causes things to not work with no error
3 participants