-
Notifications
You must be signed in to change notification settings - Fork 58
[nexus] Add TUF repo list endpoint, make other endpoints conventional #9106
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
base: main
Are you sure you want to change the base?
Conversation
}) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda thought I already had this, but I guess not. We can sort by version because we store the version in the DB as a lexicographically-sortable string. Thanks, 2023 me.
omicron/nexus/db-model/src/semver_version.rs
Lines 48 to 65 in 504ffcd
/// Pad the version numbers with zeros so the result is lexicographically | |
/// sortable, e.g., `0.1.2` becomes `00000000.00000001.00000002`. | |
/// | |
/// This requires that we impose a maximum size on each of the numbers so as not | |
/// to exceed the available number of digits. | |
/// | |
/// An important caveat is that while lexicographic sort with padding does work | |
/// for the numeric part of the version string, it does not technically satisfy | |
/// the semver spec's rules for sorting pre-release and build metadata. Build | |
/// metadata is supposed to be ignored. Pre-release has more complicated rules, | |
/// most notably that a version *with* a pre-release string on it has lower | |
/// precedence than one *without*. See: <https://semver.org/#spec-item-11>. We | |
/// have decided sorting these wrong is tolerable for now. We can revisit later | |
/// if necessary. | |
/// | |
/// Compare to the `Display` implementation on `Semver::Version` | |
/// <https://github.com/dtolnay/semver/blob/7fd09f7/src/display.rs> | |
fn to_sortable_string(v: &semver::Version) -> Result<String, external::Error> { |
} | ||
|
||
/// List artifacts for a specific TUF repository by system version. | ||
pub async fn tuf_repo_artifacts_list_by_version( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only used in the integration test, since we no longer have the artifacts when we fetch a repo
), | ||
// get doesn't use the query param but it doesn't break if it's there | ||
AllowedMethod::Get | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to put these in two separate entries so I could leave the query param off the get URL, but it doesn't work because it wants all calls to a given path in one entry -- it expected GET to 405 when it was testing the PUT by itself.
46f7ecd
to
1c34122
Compare
/// with wicket, we read the file contents from stdin so we don't know the | ||
/// correct file name). | ||
pub file_name: String, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just the fields that were already on TufRepoMeta
, but I haven't really thought about whether they're all necessary. I have no idea what targets_role_version
is, so there's at least one candidate for removal. I also will probably want to explain each field better.
These are breaking API changes and I won't merge until I have a PR up on the CLI side that makes this all work.
Add list endpoint and fix other ones
Made the endpoints match the other ones. Not married to the verb
upload
, butsystem_update_repository_update
felt a little silly.Standardize and simplify response types
What's responsible for this PR being so big is that I reworked these endpoints to return a new
TufRepo
struct that is only the metadata and no artifacts. The main reason for this was that the list endpoint would have to pull artifacts for every repo in the list. The list is likely to be small, so it's probably not a big deal, but it also seems that the artifacts are not useful to the end user. Once the list endpoint was simplified, it makes sense to return the same thing from the view and update endpoints. The upload endpoint returns aTufRepoUpload
which has theTufRepo
together with an indicator saying whether the repo contents were new repo or already existed.