-
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
Conversation
Move operator-controller APIs into apis/operator-controller Move catalogd APIs into apis/catalogd There's a conflict in types, both have a CatalogSource type that we may not be able to easily rename, so the simplest solution was to keep the two APIs serparate Signed-off-by: Todd Short <[email protected]>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1800 +/- ##
==========================================
- Coverage 68.34% 68.32% -0.02%
==========================================
Files 63 63
Lines 5117 5117
==========================================
- Hits 3497 3496 -1
- Misses 1390 1391 +1
Partials 230 230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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) |
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.
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
--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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to consolidate all.
They are the same API group.
I do not think we should have the catalogd and operator-controller dir at all.
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.
/lgtm
Because we are moving the dir the check We need to disable it, merge and then enable :-( |
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.
/lgtm cancel
We need to discuss the path for the API
As I said, I'm working on a fixup to combine it all into |
Given some decisions about the code layout from here: #1707 (comment) It seems like
combined Makefile etc. |
8fd4464
Move operator-controller APIs into apis/operator-controller Move catalogd APIs into apis/catalogd
There's a conflict in types, both have a CatalogSource type that we may not be able to easily rename, so the simplest solution was to keep the two APIs serparate
Description
Reviewer Checklist