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

Support pulling OCI Image Index manifests #3715

Merged

Conversation

rquinio
Copy link
Contributor

@rquinio rquinio commented Jul 28, 2022

Resolves #2749

Implementation follows #3700 (comment)

  • Add Accept header application/vnd.oci.image.index.v1+json during base image pull
  • Re-use the logic from V22ManifestListTemplate to select the target platform diget via a new interface ManifestListTemplate

Before filing a pull request, make sure to do the following:

This helps to reduce the chance of having a pull request rejected.

@elefeint
Copy link
Contributor

@rquinio Thank you; we'll review next week.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

The PR looks very good; I apologize for the delay.

Could you run ./gradlew goJF to reformat the code (details? That should fix the code coverage by allowing tests to run.

@rquinio
Copy link
Contributor Author

rquinio commented Aug 13, 2022

No worries. Hopefully I've fixed the formatting issue.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Could you add tests for:

  1. AbstractManifestPuller.getAccept() returning the correct media type for OciIndexTemplate.class
  2. `AbstractManifestPuller.getManifestTemplateFromJson() returning the right thing

Thank you!

@rquinio
Copy link
Contributor Author

rquinio commented Aug 20, 2022

Done ! I also took the opportunity to fix missing coverage in PullBaseImageStep for manifest lists pulling and case where the target architecture is not in the list.

@elefeint
Copy link
Contributor

Test failure:

> Task :jib-core:test

com.google.cloud.tools.jib.builder.steps.PullBaseImageStepTest > testCall_allMirrorsFail FAILED
    java.lang.NullPointerException
        at com.google.cloud.tools.jib.builder.steps.PullBaseImageStep.pullBaseImages(PullBaseImageStep.java:296)
        at com.google.cloud.tools.jib.builder.steps.PullBaseImageStep.call(PullBaseImageStep.java:159)
        at com.google.cloud.tools.jib.builder.steps.PullBaseImageStepTest.testCall_allMirrorsFail(PullBaseImageStepTest.java:571)

com.google.cloud.tools.jib.builder.steps.PullBaseImageStepTest STANDARD_OUT
    [MockitoHint] PullBaseImageStepTest.testCall_allMirrorsFail (see javadoc for MockitoHint):
    [MockitoHint] 1. Unused... -> at com.google.cloud.tools.jib.builder.steps.PullBaseImageStepTest.setUpWorkingRegistryClientFactoryWithV22ManifestList(PullBaseImageStepTest.java:672)
    [MockitoHint]  ...args ok? -> at com.google.cloud.tools.jib.builder.steps.PullBaseImageStep.pullBaseImages(PullBaseImageStep.java:294)

@rquinio
Copy link
Contributor Author

rquinio commented Aug 23, 2022

Should be fixed, testCall_allMirrorsFail was wrongly using the client factory with new V22ManifestList mocks, instead of the old V22ManifestTeplate mock.

@elefeint
Copy link
Contributor

@rquinio Thank you! I'm afraid we need a bit more testing to pass the 80% coverage gate. OciIndexTemplate.ManifestDescriptorTemplate is likely an easy candidate. Can you see the Sonar report that's linked in "Details" under the Github Action checks?

- Add Accept header application/vnd.oci.image.index.v1+json during base image pull
- Re-use the logic from V22ManifestListTemplate to select the target platform diget via a new interface ManifestListTemplate
@rquinio
Copy link
Contributor Author

rquinio commented Aug 24, 2022

@elefeint I've added a test for POJO -> JSON serialization.
I had a look at the code duplication, but I wouldn't risk factorizing the JSON objects between docker and OCI manifests as they have different fields, and I'm not sure of the details of each spec.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Thanks! I agree that the two formats could easily diverge in the future.

@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

95.2% 95.2% Coverage
4.5% 4.5% Duplication

@elefeint elefeint merged commit 7fb3031 into GoogleContainerTools:master Aug 25, 2022
@rquinio rquinio deleted the feature/pull-oci-index branch August 26, 2022 18:47
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.

Enable accepting OCI image index for multi-platform image building
3 participants