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.

Sign up for GitHub

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

Jump to bottom

Bug 1705752: update crd #319

Closed

dmage wants to merge 5 commits into openshift:master from dmage:update-crd

Contributor

dmage commented Jul 12, 2019

No description provided.

openshift-ci-robot requested review from coreydaley and gabemontero

July 12, 2019 13:54

openshift-ci-robot added the size/XXL label

Contributor

openshift-ci-robot commented Jul 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dmage]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the approved label

Contributor Author

dmage commented Jul 12, 2019

/hold

waiting until GCS is landed

openshift-ci-robot added the do-not-merge/hold label

Contributor Author

dmage commented Jul 16, 2019

/hold cancel

openshift-ci-robot removed the do-not-merge/hold label

dmage force-pushed the update-crd branch from 68b1f6f to 3543540 Compare

July 16, 2019 14:13

adambkaplan suggested changes

View reviewed changes

Contributor

adambkaplan left a comment

Looks like with our current form, the CRD is so restrictive that I doubt any sample config will work.

manifests/00-crd.yaml

+                                            type: string
+                                          name:
+                                            description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+                                              TODO: Add other useful fields. apiVersion, kind, uid?'

Contributor

adambkaplan Jul 16, 2019

Need to clean up our doc comments - this will show up in oc explain

Contributor Author

dmage Jul 17, 2019

This is a part of k8s.io/api/core/v1.

Contributor

adambkaplan Jul 19, 2019

Something that we can do upstream!

Contributor

adambkaplan Jul 19, 2019

(obviously won't address in this PR)

manifests/00-crd.yaml

+                                    - baseURL
+                                    - duration
+                                    - keypairID
+                                    - privateKey

Contributor

adambkaplan Jul 16, 2019

Looks like our CRD generator is interpreting everything as required. API needs to be updated to have // +optional in the doc comment and omitempty in the json tag.

Refer to upstream api doc, types may also need to be switched to pointers - https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Contributor Author

dmage Jul 17, 2019

// +optional is enough. I added this comment tag to the optional fields.

openshift-ci-robot added the needs-rebase label

Oleg Bulatov added 5 commits

July 17, 2019 16:48


          Cleanup required packages in Gopkg.toml

cff0d74

This field should contain only packages that are used in the project
without importing them (for example, in shell scripts).


          Update crd-schema-gen

5f9c579


          Add +optional comment tags

7d1824a


          bump(*)

97f8f0f


          make generate

4a35243

dmage force-pushed the update-crd branch from 94dde25 to 4a35243 Compare

July 17, 2019 14:52

openshift-ci-robot removed the needs-rebase label

Contributor Author

dmage commented Jul 18, 2019

/assign @adambkaplan

openshift-ci-robot assigned adambkaplan

adambkaplan suggested changes

View reviewed changes

Contributor

adambkaplan left a comment

A few nits so we play nicer with the crd-gen. Also raised a few questions that need to be addressed by api-review

pkg/apis/imageregistry/v1/types.go

-              	// Proxy defines the proxy to be used when calling master api, upstream registries, etc
+              	// Proxy defines the proxy to be used when calling master api, upstream
+              	// registries, etc.
+              	// +optional
               	Proxy ImageRegistryConfigProxy `json:"proxy"`

Contributor

adambkaplan Jul 19, 2019

Need a pointer here so we can distinguish between "no proxy was set" vs. "don't use a proxy for the registry".

Suggested change

      
            	Proxy ImageRegistryConfigProxy `json:"proxy"`
          
            	Proxy *ImageRegistryConfigProxy `json:"proxy,omitempty"`

Contributor Author

dmage Jul 19, 2019

Do you want this PR to change the logic for proxies? Or should this PR depend on your PR about a generic Proxy?

Contributor Author

dmage Jul 24, 2019

pkg/apis/imageregistry/v1/types.go

-              	// Requests controls how many parallel requests a given registry instance will handle before queuing additional requests
+              	// Requests controls how many parallel requests a given registry instance
+              	// will handle before queuing additional requests.
+              	// +optional
               	Requests ImageRegistryConfigRequests `json:"requests"`

Contributor

adambkaplan Jul 19, 2019

Pointer+omitempty - otherwise you'll see requests: {} in the YAML output.

Suggested change

      
            	Requests ImageRegistryConfigRequests `json:"requests"`
          
            	Requests *ImageRegistryConfigRequests `json:"requests,omitempty"`

pkg/apis/imageregistry/v1/types.go

+              	HTTP string `json:"http"`
+              	// +optional
+              	HTTPS string `json:"https"`
+              	// +optional

Contributor

adambkaplan Jul 19, 2019

If we make Proxy a pointer in spec, then we can can keep these as "required".

I will also be adding a PR soon to have a generic Proxy with these fields (and appropriate doc for oc explain)

pkg/apis/imageregistry/v1/types.go

@@ @@ -176,7 +184,8 @@ type ImageRegistryConfigStorageS3CloudFront struct { @@
               	PrivateKey corev1.SecretKeySelector `json:"privateKey"`
               	// KeypairID is key pair ID provided by AWS.
               	KeypairID string `json:"keypairID"`
-              	// Duration is the duration of the Cloudfront session (optional).
+              	// Duration is the duration of the Cloudfront session.
+              	// +optional
               	Duration metav1.Duration `json:"duration"`

Contributor

adambkaplan Jul 19, 2019 •

edited

Loading

Do we want omitempty - otherwise we'll get duration: 0s in the YAML output?

pkg/apis/imageregistry/v1/types.go

+              	// Bucket is the bucket name in which you want to store the registry's
+              	// data.
+              	// Optional, will be generated if not provided.
+              	// +optional
               	Bucket string `json:"bucket"`

Contributor

adambkaplan Jul 19, 2019

Convert to pointer + omitempty?

pkg/apis/imageregistry/v1/types.go

-              	// Optional, will be set based on the installed AWS Region
+              	// Region is the AWS region in which your bucket exists.
+              	// Optional, will be set based on the installed AWS Region.
+              	// +optional
               	Region string `json:"region"`

Contributor

adambkaplan Jul 19, 2019

@openshift/api-reviewers does it make sense to have an optional field that is not omitempty?

pkg/apis/imageregistry/v1/types.go

+              	// +optional
+              	TenantID string `json:"tenantID"`
+              	// +optional
+              	RegionName string `json:"regionName"`

Contributor

adambkaplan Jul 19, 2019

Needs doc for oc explain

pkg/apis/imageregistry/v1/types.go

               }
               type ImageRegistryConfigStoragePVC struct {
+              	// +optional

Contributor

adambkaplan Jul 19, 2019

Needs doc for oc explain

pkg/apis/imageregistry/v1/types.go

@@ @@ -255,47 +289,54 @@ type ImageRegistryConfigStorage struct { @@
               	// for production use. When the pod is removed from a node for any reason,
               	// the data in the emptyDir is deleted forever.
               	// This configuration is EXPERIMENTAL and is subject to change without notice.
+              	// +optional

Contributor

adambkaplan Jul 19, 2019

We need to add a discriminator field and type, i.e.

type ImageRegistryConfigStorageType string

const (
    AzureImageRegistryStorageType ImageRegistryConfigStorageType = "azure"
    GCSImageRegistryStorageType ImageRegistryConfigStorageType = "gcs"
    S3ImageRegistryStorageType ImageRegistryConfigStorageType = "s3"
...
)

And in the storage struct:

    Type ImageRegistryConfigStorageType `json:type`

See openshift/api#300

Contributor Author

dmage Jul 19, 2019

Apparently we also need to change our logic to use this field. And we should do it backward compatible. Should it be a separate PR?

Contributor Author

dmage Jul 24, 2019

Contributor

openshift-ci-robot commented Jul 20, 2019

@dmage: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the needs-rebase label

dmage changed the title ~~Update crd~~ Bug 1705752: update crd

openshift-ci-robot added the bugzilla/invalid-bug label

Contributor

openshift-ci-robot commented Jul 22, 2019

@dmage: This pull request references an invalid Bugzilla bug:

expected dependent Bugzilla bug to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ASSIGNED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1705752: update crd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Contributor Author

dmage commented Jul 24, 2019

/bugzilla refresh

openshift-ci-robot added the bugzilla/valid-bug label

Contributor

openshift-ci-robot commented Jul 24, 2019

@dmage: This pull request references a valid Bugzilla bug. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot removed the bugzilla/invalid-bug label

adambkaplan mentioned this pull request

Bug 1705752: Update crd #331

Merged

Contributor

adambkaplan commented Jul 26, 2019

/close

Replaced by #331

Contributor

openshift-ci-robot commented Jul 26, 2019

@adambkaplan: Closed this PR.

In response to this:

/close

Replaced by #331

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved bugzilla/valid-bug needs-rebase size/XXL