-
Notifications
You must be signed in to change notification settings - Fork 64
✨ OPRUN-3763: Consolidate api directory #1800
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,10 +120,10 @@ KUSTOMIZE_OPCON_CRDS_DIR := config/base/operator-controller/crd/bases | |
KUSTOMIZE_OPCON_RBAC_DIR := config/base/operator-controller/rbac | ||
manifests: $(CONTROLLER_GEN) #EXHELP Generate WebhookConfiguration, ClusterRole, and CustomResourceDefinition objects. | ||
# Generate the operator-controller manifests | ||
rm -rf $(KUSTOMIZE_OPCON_CRDS_DIR) && $(CONTROLLER_GEN) crd paths=./api/... output:crd:artifacts:config=$(KUSTOMIZE_OPCON_CRDS_DIR) | ||
rm -rf $(KUSTOMIZE_OPCON_CRDS_DIR) && $(CONTROLLER_GEN) crd paths=./api/operator-controller/... output:crd:artifacts:config=$(KUSTOMIZE_OPCON_CRDS_DIR) | ||
rm -f $(KUSTOMIZE_OPCON_RBAC_DIR)/role.yaml && $(CONTROLLER_GEN) rbac:roleName=manager-role paths=./internal/operator-controller/... output:rbac:artifacts:config=$(KUSTOMIZE_OPCON_RBAC_DIR) | ||
# Generate the catalogd manifests | ||
rm -rf $(KUSTOMIZE_CATD_CRDS_DIR) && $(CONTROLLER_GEN) crd paths="./catalogd/api/..." output:crd:artifacts:config=$(KUSTOMIZE_CATD_CRDS_DIR) | ||
rm -rf $(KUSTOMIZE_CATD_CRDS_DIR) && $(CONTROLLER_GEN) crd paths=./api/catalogd/... output:crd:artifacts:config=$(KUSTOMIZE_CATD_CRDS_DIR) | ||
rm -f $(KUSTOMIZE_CATD_RBAC_DIR)/role.yaml && $(CONTROLLER_GEN) rbac:roleName=manager-role paths="./internal/catalogd/..." output:rbac:artifacts:config=$(KUSTOMIZE_CATD_RBAC_DIR) | ||
rm -f $(KUSTOMIZE_CATD_WEBHOOKS_DIR)/manifests.yaml && $(CONTROLLER_GEN) webhook paths="./internal/catalogd/..." output:webhook:artifacts:config=$(KUSTOMIZE_CATD_WEBHOOKS_DIR) | ||
|
||
|
@@ -358,12 +358,12 @@ API_REFERENCE_DIR := $(ROOT_DIR)/docs/api-reference | |
|
||
crd-ref-docs: $(CRD_REF_DOCS) #EXHELP Generate the API Reference Documents. | ||
rm -f $(API_REFERENCE_DIR)/$(OPERATOR_CONTROLLER_API_REFERENCE_FILENAME) | ||
$(CRD_REF_DOCS) --source-path=$(ROOT_DIR)/api \ | ||
$(CRD_REF_DOCS) --source-path=$(ROOT_DIR)/api/operator-controller \ | ||
--config=$(API_REFERENCE_DIR)/crd-ref-docs-gen-config.yaml \ | ||
--renderer=markdown --output-path=$(API_REFERENCE_DIR)/$(OPERATOR_CONTROLLER_API_REFERENCE_FILENAME); | ||
|
||
rm -f $(API_REFERENCE_DIR)/$(CATALOGD_API_REFERENCE_FILENAME) | ||
$(CRD_REF_DOCS) --source-path=$(ROOT_DIR)/catalogd/api \ | ||
$(CRD_REF_DOCS) --source-path=$(ROOT_DIR)/api/catalogd \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not simplify here as well ./... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not if the API docs are in different locations, if we wish to consolidate the documentation, then that's a possibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to consolidate all. |
||
--config=$(API_REFERENCE_DIR)/crd-ref-docs-gen-config.yaml \ | ||
--renderer=markdown --output-path=$(API_REFERENCE_DIR)/$(CATALOGD_API_REFERENCE_FILENAME); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By consolidating the API, I thunk we no longer need to generate rules webhooks and crd spllited
We can just
$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
Also, I think we need to stop to rm the files.
We should run the commands and the make verify should fail if has a diff instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a 100% consolidation, as the CRDs are still put into separate locations of the
config
to keep them with their corresponding manager, which is desirable as far as downstream is concerned.I'm looking at a fixup commit to this one that does the 100% consolidation, and it is starting out to be much bigger and complicated...
I would have to look at why/when
rm
was added, and I think it was to properly make all the files and clean up those that don't need to exist any more? But it was put there for a reason.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rm was added to ensure that we always generate it.
but for all we can either do that in a follow-up.
So, there is no reason to block this one