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

fix(topbar): support teams in topbar owner links #2432

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

Tyrubias
Copy link
Contributor

@Tyrubias Tyrubias commented Feb 29, 2024

Currently the owner links in the top bar does not support teams. This pull request updates the database to store the kind of the crate owner (currently either user or team). The owner kind is added or updated to the database when a crate is built. It also updates the top bar to render the appropriate link based on kind. Finally, this pull request adds a subcommand to update all crate data from the crates.io API.

I chose not to use an enum for the kind of the crate owner because the only place in the code which we directly need to make a decision based on the crate owner kind is in the Tera template for the top bar. Otherwise, we would need to convert the enum to and from a string every time we retrieved the data from the database. Please let me know if this is a good approach.

Closes #2356

@Tyrubias Tyrubias requested a review from a team as a code owner February 29, 2024 10:06
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Feb 29, 2024
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

awesome work, thank you for taking care of it!

I had some comments inline, and I also did a manual test:

grafik

I think while the link is correct now, we should render the team differently.

Perhaps a different icon, and shorten the team-name?
I'm trying to remember what the github way of rendering is, perhaps something like @rustcrypto/block-chipers instead of github:rustcrypto:block-ciphers ?

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Feb 29, 2024
@GuillaumeGomez
Copy link
Member

If needed, I can fix the GUI in a later PR.

@Tyrubias Tyrubias force-pushed the working-owner-links branch from eda029c to 4addf4c Compare May 2, 2024 04:49
@Tyrubias
Copy link
Contributor Author

Tyrubias commented May 2, 2024

@rustbot ready

@rustbot
Copy link
Collaborator

rustbot commented May 2, 2024

Error: The feature shortcut is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Tyrubias
Copy link
Contributor Author

Tyrubias commented May 2, 2024

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels May 2, 2024
@Tyrubias Tyrubias requested a review from syphar May 2, 2024 06:44
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

Thank you for your continued work on this!

last missing piece would be a small test for the new behaviour in the topbar.

( if you don't have time for this I can also approve & merge this and add the test myself, as the current behaviour is also not tested :) )

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 15, 2024
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

I did another manual test of this PR and decided to merge it as-is.

In separate PRs I would recommend that we:

  • add a test
  • render teams a little differently.

@syphar syphar merged commit 3ce9c61 into rust-lang:master Jun 21, 2024
25 of 26 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 21, 2024
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Jun 23, 2024
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.

owner link to team doesn't link to team
4 participants