Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Rename .spec.image.ociClaim.ref to .spec.image.oci for simplicity #311

Merged
merged 6 commits into from
Aug 12, 2019
Merged

Rename .spec.image.ociClaim.ref to .spec.image.oci for simplicity #311

merged 6 commits into from
Aug 12, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Aug 9, 2019

This changes the spec for images, kernels and VMs from

spec:
  image:
    ociClaim:
      ref: weaveworks/ignite-ubuntu:latest
      type: Docker

to

spec:
  image:
    # Any image that is following the OCI specification is valid here
    oci: weaveworks/ignite-ubuntu:latest
    # Reserve the possibility to add fields for other details

according to #309. The change basically replaces the old ociClaim format in all places it was used. This PR also implements validation and conversion for the new oci field.
Fixes #309.

cc @luxas

@twelho twelho added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to the design of the project. kind/refactor Categorizes issue or PR as related to refactoring code. kind/enhancement Categorizes issue or PR as related to improving an existing feature. labels Aug 9, 2019
@twelho twelho added this to the v0.5.0 milestone Aug 9, 2019
luxas
luxas approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM overall, but a couple of nits

pkg/apis/ignite/types.go Outdated
}

type VMKernelSpec struct {
OCIClaim OCIImageClaim `json:"ociClaim"`
CmdLine string `json:"cmdLine,omitempty"`
OCIRef meta.OCIImageRef `json:"oci"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, you always want to match the field name with the JSON tag to avoid so-called "OpenAPI violations".
Let's change this name to avoid those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 9d6ab9e.

pkg/apis/ignite/v1alpha1/conversion.go Outdated
return err
}

// Convert between the old and new OCI reference format
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe create a Convert_v1alpha1_OCIClaim_To_ignite_OCI(in *OCIClaim, out *meta.OCIImageRef) error and vice-versa which you can call in all these higher-context functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9d6ab9e.

pkg/apis/ignite/v1alpha1/conversion.go Outdated
}

// Convert between the old and new OCI reference format
out.OCIRef = in.OCIClaim.Ref
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd comment specifically here that in.OCIClaim.Type is ignored on purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added that comment to Convert_v1alpha1_OCIClaim_To_ignite_OCI in 9d6ab9e.

pkg/apis/ignite/types.go
// OCIImageClaim defines a claim for importing an OCI image
type OCIImageClaim struct {
// Type defines how the image should be imported
Type ImageSourceType `json:"type"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove the ImageSourceType enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 9d6ab9e.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed GetImageSourceTypes as it's redundant.

pkg/openapi/violations.txt Outdated
@@ -6,6 +6,10 @@ API rule violation: list_type_missing,github.com/weaveworks/ignite/pkg/apis/igni
API rule violation: list_type_missing,github.com/weaveworks/ignite/pkg/apis/ignite/v1alpha2,VMStorageSpec,VolumeMounts
API rule violation: list_type_missing,github.com/weaveworks/ignite/pkg/apis/ignite/v1alpha2,VMStorageSpec,Volumes
API rule violation: names_match,github.com/weaveworks/ignite/pkg/apis/ignite/v1alpha1,VMSpec,CPUs
API rule violation: names_match,github.com/weaveworks/ignite/pkg/apis/ignite/v1alpha2,ImageSpec,OCIRef
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name the field just OCI and these violations will go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 9d6ab9e.

@twelho
Copy link
Contributor Author

twelho commented Aug 12, 2019

Feedback addressed and CI is green, merging 👍

@twelho twelho merged commit afb8a84 into weaveworks:master Aug 12, 2019
@twelho twelho deleted the spec-oci-redesign branch August 12, 2019 09:45
@luxas luxas removed the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Aug 12, 2019
@luxas luxas changed the title OCIImageClaim -> OCIRef, simplify by naming the field oci Rename .spec.image.ociClaim.ref to .spec.image.oci for simplicity Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luxas luxas luxas approved these changes

Assignees

@luxas luxas

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to the design of the project. kind/refactor Categorizes issue or PR as related to refactoring code.
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.

Redesign the image and kernel ociClaim
2 participants
@twelho @luxas