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

Add storefront-info-endpoint configuration #65

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

jameswestman
Copy link
Collaborator

@jameswestman jameswestman commented Jun 8, 2022

If this configuration option is present, this endpoint will be used to get information about each app when it is uploaded. It can be used to validate or modify appstream files and to set the token type of the commit.

Depends on #69.

@jameswestman jameswestman force-pushed the storefront-info branch 2 times, most recently from c28bf6d to 18d3137 Compare June 8, 2022 19:07
@jameswestman jameswestman force-pushed the storefront-info branch 5 times, most recently from 96f0509 to 70b5a1a Compare July 1, 2022 19:33
@jameswestman
Copy link
Collaborator Author

This adds a dependency on libostree >=2021.5 for rewriting commits. This caused some issues with CI, so I had to change the jobs to use Ubuntu 22.04 rather than latest (which is currently 20.04).

@jameswestman jameswestman force-pushed the storefront-info branch 2 times, most recently from 0bbd049 to b025e71 Compare July 1, 2022 20:16
Copy link

@danielsilverstone-ct danielsilverstone-ct left a comment

Choose a reason for hiding this comment

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

There are a lot of places where you added an Eq derive - was this needed for the ostree stuff? In this instance it won't add a huge amount to compile times / binary size; but in general it's not a great idea to blanket-add derives unless you need them. Obvs. if you need them then so-be-it, which is why I didn't highlight them in the main review comments.

Generally this looks good; though I pretend no ability to judge the ostree work.

.github/workflows/ci.yml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/api.rs Outdated Show resolved Hide resolved
src/jobs.rs Show resolved Hide resolved
src/jobs.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/storefront.rs Show resolved Hide resolved
src/storefront.rs Outdated Show resolved Hide resolved
@jameswestman
Copy link
Collaborator Author

As for the Eq derives, that was a clippy warning--of course, it might be wrong.

Copy link

@danielsilverstone-ct danielsilverstone-ct left a comment

Choose a reason for hiding this comment

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

As someone with minimal experience of ostree etc. the Rust itself looks nice now.

@ramcq
Copy link
Contributor

ramcq commented Jul 25, 2022

Would @dbnicholson or @cgwalters be able to take a look at the ostree surgery parts of this? It's a Rust version of some utterly crazy stuff we have in Endless OS which I made for renaming Flatpaks locally, using an in-memory mtree to commit updated metadata/etc files which have the app ID changed.

Copy link

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Overall looks sane to me, just some completely optional suggestions/notes

src/jobs.rs Outdated Show resolved Hide resolved
src/jobs.rs Outdated Show resolved Hide resolved
src/jobs.rs Outdated Show resolved Hide resolved
src/jobs.rs Show resolved Hide resolved
src/jobs.rs Show resolved Hide resolved
src/jobs.rs Show resolved Hide resolved
@jameswestman
Copy link
Collaborator Author

Pushed some changes:

  • Minor code style changes (NONE_CANCELLABLE instead of ::<Cancellable>, etc.)
  • Fix unnecessary re-commits caused by re-serializing the appstream XML
  • Update ostree crate to 0.15

@barthalion
Copy link
Member

@jameswestman Mind regenerating Cargo.lock after a rebase?

If this configuration option is present, this endpoint will be used to
get information about each app when it is uploaded. It can be used to
validate or modify appstream files and to set the token type of the
commit.
@barthalion barthalion merged commit d291479 into flatpak:master Sep 19, 2022
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.

5 participants