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

rename registry clusteroperator resource plus file renames+cleanup #189

Merged
merged 1 commit into from
Feb 8, 2019

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Feb 4, 2019

  1. add clusteroperator skeleton to our manifest so the CVO respects it
  2. rename our manifests dir to "manifests" to align w/ other operators
  3. remove what appeared to be some dead files

@bparees bparees force-pushed the naming branch 2 times, most recently from bc480dc to b602d96 Compare February 4, 2019 19:03
@bparees
Copy link
Contributor Author

bparees commented Feb 4, 2019

/assign @dmage @legionus @coreydaley

@bparees
Copy link
Contributor Author

bparees commented Feb 4, 2019

/hold

we'll want to see this pass e2e-aws multiple times to ensure we're not going to block the installer w/ this change

@bparees bparees added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2019
@bparees
Copy link
Contributor Author

bparees commented Feb 4, 2019

/assign @dmage @legionus @coreydaley
(bot was dead)

@bparees
Copy link
Contributor Author

bparees commented Feb 4, 2019

/refresh

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 4, 2019
@bparees
Copy link
Contributor Author

bparees commented Feb 5, 2019

/test e2e-aws

@bparees
Copy link
Contributor Author

bparees commented Feb 5, 2019

/retest

@bparees
Copy link
Contributor Author

bparees commented Feb 5, 2019

@smarterclayton @deads2k @abhinavdahiya i'm trying to add the image-registry clusteroperator resource to the manifest, but the CVO seems to be having issues picking it up:

I0204 23:47:22.558954       1 sync_worker.go:442] Running sync for clusteroperator "image-registry" (config.openshift.io/v1, 177 of 261)
E0204 23:47:23.563157       1 task.go:57] error running apply for clusteroperator "image-registry" (config.openshift.io/v1, 177 of 261): clusteroperators.config.openshift.io "image-registry" not found
E0204 23:47:48.583150       1 sync_worker.go:266] unable to synchronize image (waiting 3m19.747206386s): Could not update clusteroperator "image-registry" (config.openshift.io/v1, 177 of 261): resource may have been deleted

the CRD was defined here:

I0204 23:38:11.787638       1 sync_worker.go:468] Done syncing for customresourcedefinition "clusteroperators.config.openshift.io" (apiextensions.k8s.io/v1beta1, 2 of 261)

and other cluster operator resources are managed w/o issue:

I0204 23:47:17.932566       1 sync_worker.go:468] Done syncing for clusteroperator "kube-apiserver" (config.openshift.io/v1, 56 of 261)

and there is no sign the registry operator was actually created (would normally happen after the clusteroperator resource was created based on our resource ordering), so i don't know what else would be deleting/interacting with that resource.

the full CVO log is here:
https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-image-registry-operator/189/pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws/991/artifacts/e2e-aws/pods/openshift-cluster-version_cluster-version-operator-647b8f5746-tc84r_cluster-version-operator.log.gz

Is it invalid to create the clusteroperator resource before creating the operator (because the CVO is going to wait for the clusteroperator to report available before instantiating the rest of the manifest resources)? Still wouldn't explain why it's reported as removed though..

@bparees
Copy link
Contributor Author

bparees commented Feb 5, 2019

/retest
/test e2e-aws

1 similar comment
@bparees
Copy link
Contributor Author

bparees commented Feb 5, 2019

/retest
/test e2e-aws

@bparees
Copy link
Contributor Author

bparees commented Feb 6, 2019

/retest

@bparees
Copy link
Contributor Author

bparees commented Feb 6, 2019

@dmage @legionus @coreydaley

still needs review please

@coreydaley
Copy link
Member

/lgtm
Only comment would be that I thought the style guide for golang package aliases was to use the last section of the import path, if that collides with something else, then go back one more section and so on and so forth until it is unique.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, coreydaley

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:

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

@bparees
Copy link
Contributor Author

bparees commented Feb 6, 2019

Only comment would be that I thought the style guide for golang package aliases was to use the last section of the import path, if that collides with something else, then go back one more section and so on and so forth until it is unique.

Assuming you mean stuff like this:
configapiv1 "github.com/openshift/api/config/v1"

I picked that pattern up elsewhere in origin (though i'm sure it's not consistent). In general I try to name the aliases in such a way that the references in the code make sense. (ie when you're reading the code, "configv1.Foo" doesn't tell you as much as "configapiv1.Foo" does. And don't get me started on "v1.Foo")

Also since you can have a config client v1 and a config api v1, i didn't want to just call this "configv1" and then later when someone also imports client we'd have "clientconfigv1" and "configv1" which makes it unclear to someone reading the code who just sees "configv1" what it is. So better to head off the ambiguity up front, imho, because no one's going to refactor configv1 to apiconfigv1 when they add clientconfigv1.

as for "apiconfigv1" vs "configapiv1", the latter reads more naturally to me (and again, is what i've seen elsewhere in origin).

tldr: i prefer readability/clarity over arbitrary style guide recommendations

@dmage
Copy link
Member

dmage commented Feb 6, 2019

Despite the fact that I agree with the reasoning, I don't like the ...apiv1 approach just because it's inconsistent with corev1 and metav1. And these values I'd like to not to change, because they are used everywhere.

@bparees
Copy link
Contributor Author

bparees commented Feb 6, 2019

Despite the fact that I agree with the reasoning, I don't like the ...apiv1 approach just because it's inconsistent with corev1 and metav1. And these values I'd like to not to change, because they are used everywhere.

we can certainly retain an exception for corev1 and metav1, I agree that I don't want to go search+replace the world. I'm not even suggesting we go change any of our existing ones, but as I find them in files i'm working on, I try to update them.

(example of the pattern i'm trying to follow from origin:
https://github.com/openshift/origin/blob/master/pkg/api/imagereferencemutators/internalversion/pods.go#L19-L22)

(note that they used "kapiv1" for corev1 in that file, so it's all an inconsistent mess anyway:
https://github.com/openshift/origin/blob/master/pkg/api/imagereferencemutators/internalversion/pods.go#L9)

@openshift-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2019
@bparees bparees closed this Feb 7, 2019
@bparees bparees reopened this Feb 7, 2019
@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/refresh

@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/test e2e-aws

3 similar comments
@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/test e2e-aws

@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/test e2e-aws

@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/test e2e-aws

@bparees
Copy link
Contributor Author

bparees commented Feb 7, 2019

/hold cancel

@bparees bparees added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 7, 2019
@openshift-merge-robot openshift-merge-robot merged commit 5430dbb into openshift:master Feb 8, 2019
@bparees bparees deleted the naming branch February 14, 2019 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants