-
Notifications
You must be signed in to change notification settings - Fork 227
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
Handle dyno name parsing for Fir apps #3075
Conversation
867eb78
to
38fc0e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed some unexpected behavior with this change. Or, at least, it doesn't fix something that doesn't work with fir. For apps with multiple process types, it isn't grouping them correctly, as shown here (in the below example, go-getting started is a fir app with both web and worker dynos while repositories-api-production is a cedar app):
![Screenshot 2024-11-08 at 3 09 38 PM](https://private-user-images.githubusercontent.com/879929/384547572-b7d7a4db-7abb-49ba-a1bf-03500f67a5b1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0NTM4MTYsIm5iZiI6MTczOTQ1MzUxNiwicGF0aCI6Ii84Nzk5MjkvMzg0NTQ3NTcyLWI3ZDdhNGRiLTdhYmItNDliYS1hMWJmLTAzNTAwZjY3YTViMS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxM1QxMzMxNTZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT04Yzc3MTNkZDdhNWUxNmQxNDJkNjRjYWIyNGE0MmQ2MDI2MDc4OWY5M2E5OTFjYzcwYzUxMWU0MmYxNmZhOWM2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.27AMAX-WMQR1BGcj-A1gFrv8QkNdsDqCQPnUbwH5E7U)
Also, I think there's another spot we need to address dyno name parsing:
https://github.com/heroku/cli/blob/prerelease/10.0.0-alpha/packages/cli/src/commands/spaces/ps.ts#L7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For apps with multiple process types, it isn't grouping them correctly,
i take this back, i had worker dynos set to 0. it is grouping the two different web dynos together and not labeling it as web
but i don't think that has to do with fir. anyway, i think if we just address the other processNum parsing function, we're good.
Yeah, I see. I know where the issue might be, I'll provide a fix for this. |
38fc0e9
to
ca24f18
Compare
ca24f18
to
4a33b97
Compare
Description
Here we introduce changes to correctly parse dyno names for both Cedar & Fir generation apps, updating requests to use the
3.sdk
variant.SOC2
GUS Work Item