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

feat: add inspectManifest API endpoint #6812

Merged
merged 2 commits into from Apr 18, 2024
Merged

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Apr 16, 2024

feat: add inspectManifest API endpoint

What does this PR do?

  • Adds the inspectManifest API endpoint so we can retrieve information
    regarding a manifest from the podman libpodApi endpoint

Screenshot / video of UI

N/A, it's an API call.

What issues does this PR fix or reference?

Closes #6793

How to test this PR?

  • Tests are covering the bug fix or the new feature

Tests cover, otherwise you could also put the following (after creating
ANY manifest, in a svelte file:)

  // List all the images
  const images = await window.listImages();

  // Get all the ones that have isManifest set to true
  const manifestImages = images.filter(image => image.isManifest);

  // For each manifestImages use inspectManifest to get the information (passing in engineId as well)
  const imageInfoPromises = manifestImages.map(image => window.inspectManifest(image.engineId, image.Id));

  // Wait for all the promises to resolve
  const imageInfos = await Promise.all(imageInfoPromises);

  // Consoel log each one
  imageInfos.forEach(imageInfo => {
    console.log(imageInfo);
  });

Signed-off-by: Charlie Drage charlie@charliedrage.com

@cdrage cdrage requested review from benoitf and a team as code owners April 16, 2024 13:00
@cdrage cdrage requested review from feloy and lstocchi and removed request for a team April 16, 2024 13:00
cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 16, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->
cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 16, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Codewise LGTM

Comment on lines 38 to 43
architecture: string;
features: string[];
os: string;
osFeatures: string[];
osVersion: string;
variant: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these params mandatory ? AFAIK some of them are optional no ? like OSFeatures

Copy link
Contributor Author

@cdrage cdrage Apr 17, 2024

Choose a reason for hiding this comment

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

it is just returning information, so they are not mandatory, but no need to set them to optional as it is just there so the API can pick it up.

Copy link
Collaborator

@benoitf benoitf Apr 17, 2024

Choose a reason for hiding this comment

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

I tried with docker.io/httpd image

curl --unix-socket /var/run/docker.sock  "http://libpod/v3.0.0/libpod/manifests/docker.io%2Fhttpd/json" | jq .

the fields are not there so it should be listed as optional.

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 2095,
      "digest": "sha256:6834aeaa0023c674c53a8891ce77a363579ae9a9679222f9634f95aa72c5781b",
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 841,
      "digest": "sha256:d8081c32306039fe31141c53642d73c59678f70678e85f7506c01d15aa5a95e3",
      "platform": {
        "architecture": "unknown",
        "os": "unknown"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 2097,
      "digest": "sha256:623ea480a45a037db310321eb5f9b933df664701f5be02feb2164b302b2e293d",
      "platform": {
        "architecture": "arm",
        "os": "linux",
        "variant": "v5"
      }
    },

Comment on lines 34 to 43
manifests: {
digest: string;
mediaType: string;
platform: {
architecture: string;
features: string[];
os: string;
osFeatures: string[];
osVersion: string;
variant: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks it's not as the other Types returned with the API where it is starting by uppercase (like Architecture, OS, Manifests, etc)

Copy link
Contributor Author

@cdrage cdrage Apr 17, 2024

Choose a reason for hiding this comment

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

that's correct, unlike the other API calls throughout the podman API, manifests are all lowercase, see: https://docs.podman.io/en/latest/_static/api.html#tag/manifests/operation/ManifestInspectLibpod

vs something else like inspect pod: https://docs.podman.io/en/latest/_static/api.html#tag/pods/operation/PodInspectLibpod

Copy link
Collaborator

Choose a reason for hiding this comment

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

on this link it's listed as:

"os.features": ["string"],
"os.version": "string",

but here you introduced osFeatures and osVersion

cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 17, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage cdrage marked this pull request as draft April 17, 2024 21:42
@cdrage
Copy link
Contributor Author

cdrage commented Apr 17, 2024

@benoitf Thanks for the review, I'm going to put this back on draft as I have missed a few things

@benoitf
Copy link
Collaborator

benoitf commented Apr 17, 2024

@cdrage also maybe podman api doc is incorrect and it's not os.features but osFeatures

### What does this PR do?

* Adds the inspectManifest API endpoint so we can retrieve information
  regarding a manifest from the podman libpodApi endpoint

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A, it's an API call.

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#6793

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

Tests cover, otherwise you could also put the following (after creating
ANY manifest, in a svelte file:)

```ts
  // List all the images
  const images = await window.listImages();

  // Get all the ones that have isManifest set to true
  const manifestImages = images.filter(image => image.isManifest);

  // For each manifestImages use inspectManifest to get the information (passing in engineId as well)
  const imageInfoPromises = manifestImages.map(image => window.inspectManifest(image.engineId, image.Id));

  // Wait for all the promises to resolve
  const imageInfos = await Promise.all(imageInfoPromises);

  // Consoel log each one
  imageInfos.forEach(imageInfo => {
    console.log(imageInfo);
  });
```

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage cdrage marked this pull request as ready for review April 18, 2024 13:24
@cdrage
Copy link
Contributor Author

cdrage commented Apr 18, 2024

Ready for review again @benoitf @lstocchi @feloy

For @benoitf : I have removed the osFeatures and osVersion as they are rarely used as per: https://github.com/containers/podman/blob/01d0f88a100ca91cd7a60c691dcbba2ab29903dd/docs/source/markdown/podman-manifest-annotate.1.md.in#L38 as well as the fact that they have an uncommon json output (os.features instead of osFeatures) which doesn't match the rest of the API, so it's safe to say they are not used very often.

If we get a feature request, we should add them in.

Updated the PR & tests.

cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 18, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage cdrage merged commit 1cb85ae into containers:main Apr 18, 2024
10 checks passed
cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 18, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 18, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@podman-desktop-bot podman-desktop-bot added this to the 1.10.0 milestone Apr 18, 2024
cdrage added a commit to cdrage/podman-desktop-extension-bootc that referenced this pull request Apr 19, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
deboer-tim pushed a commit to containers/podman-desktop-extension-bootc that referenced this pull request Apr 22, 2024
### What does this PR do?

* Adds the inspectManifest endpoint to the API
* Requires containers/podman-desktop#6812 to go in
first

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes #340

### How to test this PR?

Use the `inspectManifest` endpoint with a passed in image.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
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.

Add inspectManifest API endpoint
4 participants