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

Bug fix: fly launch was not checking to see if directory name was unique #3637

Merged
merged 1 commit into from
Jun 15, 2024

Conversation

rubys
Copy link
Contributor

@rubys rubys commented Jun 14, 2024

No description provided.

@rubys rubys requested a review from alichay June 15, 2024 22:11
@alichay
Copy link
Contributor

alichay commented Jun 15, 2024

We can merge this to fix the bug, but I'm leaving this as more of a note to self than anything:

we have to validate the app name after the "plan" is constructed, which means that right now this will validate a good name twice. this makes it decently easy for normal usage to run against the rate limit for the name validation API.

@rubys
Copy link
Contributor Author

rubys commented Jun 15, 2024

I'll argue that it doesn't make sense to show a plan that is unlikely to succeed. This implies to me that we need to validate the name before we show the plan. Perhaps we only need to validate it a second time if it is changed by the launch UI?

This applies to other parts of the plan. I'm seeing a fair number of region-restricted fly launch failures; perhaps we shouldn't suggest a region unless it is available using the user's current plan?

@rubys rubys merged commit e1491ed into master Jun 15, 2024
34 checks passed
@rubys rubys deleted the find-unique-appname branch June 15, 2024 22:28
@alichay
Copy link
Contributor

alichay commented Jun 15, 2024

I'll argue that it doesn't make sense to show a plan that is unlikely to succeed. This implies to me that we need to validate the name before we show the plan. Perhaps we only need to validate it a second time if it is changed by the launch UI?

no yeah I wasn't arguing whether this was right or not haha, it definitely is

I was just saying that it'll need to remember that the name's already been validated (if it has been) so it's not repeating itself, but that can come later :) thanks for getting this fixed, esp on a weekend!!

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.

None yet

2 participants