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

Use @rustls-benchmarking as the app's name #24

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Use @rustls-benchmarking as the app's name #24

merged 1 commit into from
Dec 11, 2023

Conversation

aochagavia
Copy link
Collaborator

With this change, the name matches the real name of the app that is currently installed on the rustls repo.

@cpu I've chosen to keep the name hardcoded, even though it would be possible to obtain the application's name dynamically using the GitHub API. We aren't planning to deploy multiple instances of this, with different names each, so I'd rather go with this pragmatic choice instead of making things dynamic. If you disagree, let me know and we can discuss (your judgement weights heavier than mine here, IMO, because you are going to actually maintain this the coming months).

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

The idea of hardcoding the name vs acquiring it programmatically makes sense to me.

WDYT about defining a top-level const somewhere instead of inlining the value throughout so that at the very least if we do decide to change it, or if we end up wanting a second instance, it would be less work to find/replace?

@aochagavia
Copy link
Collaborator Author

Good suggestion, I just pushed an updated commit that uses a static string for the app name and will merge once the CI finishes

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks!

@aochagavia aochagavia merged commit d38c9f4 into main Dec 11, 2023
9 checks passed
@aochagavia aochagavia deleted the app-rename branch December 11, 2023 15:03
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