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

Runners not unregistering due to camelCase #585

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

DavidGOrtega
Copy link
Contributor

GH api uses snake_case 🐍 due to this GH are not unregistering successfully anymore

Fixed in #583 however it might take longer and this is important.

@DavidGOrtega DavidGOrtega temporarily deployed to internal June 10, 2021 12:15 Inactive
@DavidGOrtega DavidGOrtega self-assigned this Jun 10, 2021
@DavidGOrtega DavidGOrtega added cml-runner Subcommand p0-critical Max priority (ASAP) labels Jun 10, 2021
src/drivers/github.js Outdated Show resolved Hide resolved
src/drivers/github.js Outdated Show resolved Hide resolved
src/drivers/github.js Outdated Show resolved Hide resolved
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

What if we delay the conversion to snake_case until the invocation of the GitHub API? Anyhow, it might be a bit more verbose, so I don't have a strong preference over any of the methods.

@DavidGOrtega DavidGOrtega temporarily deployed to internal June 10, 2021 12:21 Inactive
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@DavidGOrtega
Copy link
Contributor Author

What if we delay the conversion to snake_case until the invocation of the GitHub API? Anyhow, it might be a bit more verbose, so I don't have a strong preference over any of the methods.

Its ugly, normally in modern js you destructure looking to match the needed member later on. In this case Im going to reuse id straight to avoid lint but in the future we should destructure snake_case and add a lint ignore rule

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

I also prefer the "ugly but correct/consistent" way suggested by @0x2b3bfa0 and used in the rest of the codebase (and my Python bias avoids variables called id) but whatever

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Jun 10, 2021

In this case was done just because is easy to read. If it were 20 lines I would not have done it either. But here is totally fine because you use it afterwards assignment

Indeed just after

const { id } = await this.runnerByName({ name });

So you understand that the id is the runner's id

@DavidGOrtega
Copy link
Contributor Author

unless your memory is worse than mine 👴

@DavidGOrtega DavidGOrtega merged commit 02dff90 into master Jun 11, 2021
@DavidGOrtega DavidGOrtega deleted the hotfix/runners-not-unregistering-in-GH branch June 11, 2021 13:32
@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-runner Subcommand p0-critical Max priority (ASAP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants