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

Handle dyno name parsing for Fir apps #3075

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

sbosio
Copy link
Contributor

@sbosio sbosio commented Nov 6, 2024

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

@sbosio sbosio temporarily deployed to AcceptanceTests November 6, 2024 22:54 — with GitHub Actions Inactive
@sbosio sbosio temporarily deployed to AcceptanceTests November 6, 2024 22:54 — with GitHub Actions Inactive
@sbosio sbosio temporarily deployed to AcceptanceTests November 6, 2024 23:05 — with GitHub Actions Inactive
@sbosio sbosio temporarily deployed to AcceptanceTests November 6, 2024 23:05 — with GitHub Actions Inactive
@eablack eablack marked this pull request as ready for review November 8, 2024 22:16
@eablack eablack requested a review from a team as a code owner November 8, 2024 22:16
@eablack eablack force-pushed the sbosio/process-type-parsing branch from 867eb78 to 38fc0e9 Compare November 8, 2024 22:17
@eablack eablack temporarily deployed to AcceptanceTests November 8, 2024 22:17 — with GitHub Actions Inactive
@eablack eablack temporarily deployed to AcceptanceTests November 8, 2024 22:17 — with GitHub Actions Inactive
Copy link
Contributor

@eablack eablack left a 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

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

Copy link
Contributor

@eablack eablack left a 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.

@sbosio
Copy link
Contributor Author

sbosio commented Nov 11, 2024

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.

@sbosio sbosio force-pushed the sbosio/process-type-parsing branch from 38fc0e9 to ca24f18 Compare November 12, 2024 20:59
@sbosio sbosio temporarily deployed to AcceptanceTests November 12, 2024 20:59 — with GitHub Actions Inactive
@sbosio sbosio temporarily deployed to AcceptanceTests November 12, 2024 20:59 — with GitHub Actions Inactive
@sbosio sbosio force-pushed the sbosio/process-type-parsing branch from ca24f18 to 4a33b97 Compare November 12, 2024 21:01
@sbosio sbosio temporarily deployed to AcceptanceTests November 12, 2024 21:01 — with GitHub Actions Inactive
@sbosio sbosio temporarily deployed to AcceptanceTests November 12, 2024 21:01 — with GitHub Actions Inactive
@sbosio sbosio merged commit c0a3519 into prerelease/10.0.0-alpha Nov 12, 2024
8 checks passed
@sbosio sbosio deleted the sbosio/process-type-parsing branch November 12, 2024 21:30
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