-
Notifications
You must be signed in to change notification settings - Fork 190
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
USHIFT-631: Rearrangement of assets directory #1144
Conversation
b9e603b
to
41476da
Compare
Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>
41476da
to
fa6521f
Compare
scripts/auto-rebase/rebase.sh
Outdated
@@ -405,39 +405,39 @@ update_manifests() { | |||
# 1) Adopt resource manifests | |||
# Selectively copy in only those core manifests that MicroShift is already using | |||
cp "${STAGING_DIR}/cluster-openshift-controller-manager-operator/bindata/v3.11.0/openshift-controller-manager/ns.yaml" "${REPOROOT}"/assets/core/0000_50_cluster-openshift-controller-manager_00_namespace.yaml | |||
cp "${STAGING_DIR}/cluster-openshift-controller-manager-operator/bindata/v3.11.0/openshift-controller-manager/route-controller-ns.yaml" "${REPOROOT}"/assets/core/0000_50_cluster-openshift-route-controller-manager_00_namespace.yaml |
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.
+1 for this
scripts/auto-rebase/rebase.sh
Outdated
# Selectively copy in only those CRD manifests that MicroShift is already using | ||
cp "${REPOROOT}"/vendor/github.com/openshift/api/route/v1/route.crd.yaml "${REPOROOT}"/assets/crd | ||
cp "${STAGING_DIR}/release-manifests/0000_03_security-openshift_01_scc.crd.yaml" "${REPOROOT}"/assets/crd | ||
cp "${STAGING_DIR}/release-manifests/0000_03_securityinternal-openshift_02_rangeallocation.crd.yaml" "${REPOROOT}"/assets/crd | ||
# The following manifests are just MicroShift specific and are not present in any other OpenShift repo. | ||
# - assets/crd/authorizationv1-local-apiservice.yaml (local API service for authorization API group, needed if OpenShift API server is not present) | ||
# - assets/crd/securityv1-local-apiservice.yaml (local API service for security API group, needed if OpenShift API server is not present) | ||
# - assets/apiservice/securityv1-local-apiservice.yaml (local API service for security API group, needed if OpenShift API server is not present) |
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.
just curious, what happened with authorizationv1-local-apiservice.yaml
?
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.
It was removed because it caused garbage collection issues when no available API from that API group was present.
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.
hm interesting, is there an issue posted somewhere regarding this GC behaviour? This should not happen and might be fixable upstream.
"core/ingress-to-route-controller-clusterrole.yaml", | ||
"core/route-controller-informer-clusterrole.yaml", | ||
"core/route-controller-tokenreview-clusterrole.yaml", | ||
"rbac/ingress-to-route-controller-clusterrole.yaml", |
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.
since we do not always have a prefix route-controller
, I would be in favor of having even more granular approach like rbac/route-controller-manager/
to distinguish the files. But we are not doing this in our operators and is probably okay in a current state with not as many RBACs
scripts/auto-rebase/rebase.sh
Outdated
cp "${STAGING_DIR}/cluster-openshift-controller-manager-operator/bindata/v3.11.0/openshift-controller-manager/ns.yaml" "${REPOROOT}"/assets/core/0000_50_cluster-openshift-controller-manager_00_namespace.yaml | ||
cp "${STAGING_DIR}/cluster-openshift-controller-manager-operator/bindata/v3.11.0/openshift-controller-manager/route-controller-ns.yaml" "${REPOROOT}"/assets/core/0000_50_cluster-openshift-route-controller-manager_00_namespace.yaml |
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.
what is the difference between those two and why wouldn't all this go in "${REPOROOT}"/assets/controllers/route-controller-manager/
as the following ones?
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.
in here I see we are using actually route-controller-manager 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.
good catch! I'll force update the PR.
The difference between those two files are just different ns: openshift-controller-manager
and openshift-route-controller-manager
.
Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>
@fzdarsky @mangelajo this is ready to be tagged. Thanks |
/approve |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mangelajo, oglok 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 |
@oglok: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Signed-off-by: Ricardo Noriega rnoriega@redhat.com
Closes USHIFT-631