Skip to content

Load registry config to determine which API to use #813

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

Merged
merged 6 commits into from
Jun 18, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jun 4, 2020

Another step on the way towards #767, removes hardcoded https://crates.io/api urls and determines what to use based on the config in the registry index. The API is optional for a registry, so if configured to use a registry without one it will just insert empty data when building crates.

@Nemo157
Copy link
Member Author

Nemo157 commented Jun 4, 2020

(I suggest hiding whitespace changes while viewing since the api code all got indented one more level)

Copy link
Member

@jyn514 jyn514 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 modulo nits, but I need to test locally since we don't have automatic tests for builds.

@jyn514
Copy link
Member

jyn514 commented Jun 5, 2020

Built without problems (although I found some other regressions from past PRs while playing around: #817).

@pietroalbini
Copy link
Member

I'll take a look at this PR on Monday.

@pietroalbini
Copy link
Member

This looks good to me once we change the PR to set default values for any data it can't retrieve. Also, we should avoid creating multiple HTTP clients, ideally by sharing a single one even between builds.

@Nemo157 Nemo157 force-pushed the load-registry-config branch from 88992d3 to 2cbf252 Compare June 10, 2020 16:34
Copy link
Member

@jyn514 jyn514 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 modulo nit. Needs to be rebased due to conflicts.

src/index/api.rs Outdated
let (release_time, yanked, downloads) = self
.get_release_time_yanked_downloads(name, version)
.unwrap_or_else(|err| {
info!("Failed to get crate data for {}-{}: {}", name, version, err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
info!("Failed to get crate data for {}-{}: {}", name, version, err);
warn!("Failed to get crate data for {}-{}: {}", name, version, err);

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about whether this should be a warning given that not all registries might support it, but just 2 warning logs per build seems fine.

src/index/api.rs Outdated
});

let owners = self.get_owners(name).unwrap_or_else(|err| {
info!("Failed to get owners for {}-{}: {}", name, version, err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
info!("Failed to get owners for {}-{}: {}", name, version, err);
warn!("Failed to get owners for {}-{}: {}", name, version, err);

@Nemo157 Nemo157 force-pushed the load-registry-config branch from 2cbf252 to 2923594 Compare June 18, 2020 08:01
@jyn514 jyn514 merged commit a94c2cc into rust-lang:master Jun 18, 2020
@jyn514
Copy link
Member

jyn514 commented Jun 18, 2020

Thanks for working on this! What more do we have left until we can use staging.crates.io?

@Nemo157 Nemo157 deleted the load-registry-config branch May 31, 2023 23:06
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.

4 participants