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

add configmap etcd-serving-ca.kube-system #551

Conversation

sanchezl
Copy link
Contributor

@sanchezl sanchezl commented Oct 26, 2018

  • manifests: add secret etcd-client.kube-system (used to be in cluster-kube-apiserver-operator renderer)
  • docs: updated the dependency graph at docs/design/resource_dep.svg

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 26, 2018
openshift.io/component: "api"
data:
ca-bundle.crt: |
{{ .Assets | load .EtcdServingCA | indent 4 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please link me to where this gets substituted. I would expect this to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -191,6 +191,7 @@ func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*ass
"app-version-kind.yaml": []byte(bootkube.AppVersionKind),
"app-version-tectonic-network.yaml": []byte(bootkube.AppVersionTectonicNetwork),
"etcd-service.yaml": []byte(bootkube.EtcdServiceKubeSystem),
"kube-system-configmap-etcd-serving-ca.yaml": []byte(bootkube.KubeSystemConfigmapEtcdServingCA),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this? template substitution appears to happen in the stanza above, though even so I think the parameters used don't match this pull.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@deads2k
Copy link
Contributor

deads2k commented Oct 26, 2018

@sanchezl contribution requirements to this repo are different. please adjust your commit message per https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format and our agreement with @crawford and @abhinavdahiya

@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 9b36697 to 04809f8 Compare October 26, 2018 20:12
@sanchezl sanchezl changed the title move kube-system-configmap-etcd-serving-ca.yaml to installer [WIP] move kube-system-configmap-etcd-serving-ca.yaml to installer Oct 26, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2018
@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 04809f8 to b78f984 Compare October 26, 2018 21:05
@abhinavdahiya abhinavdahiya changed the title [WIP] move kube-system-configmap-etcd-serving-ca.yaml to installer [WIP] add kube-system-configmap-etcd-serving-ca.yaml Oct 26, 2018
@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from b78f984 to 62da5b2 Compare October 30, 2018 01:47
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 30, 2018
@deads2k
Copy link
Contributor

deads2k commented Oct 30, 2018

@sanchezl is this one ready now?

@sanchezl sanchezl changed the title [WIP] add kube-system-configmap-etcd-serving-ca.yaml add kube-system-configmap-etcd-serving-ca.yaml Oct 30, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2018
@wking
Copy link
Member

wking commented Oct 30, 2018

The 62da5b2 commit message says:

The renderer in cluster-kube-apiserver-operator will need to be also changed to stop creating the same manifest file.

But on a quick skim, I didn't see any PRs there making that change. Can you link us to the operator change?

@sanchezl
Copy link
Contributor Author

The 62da5b2 commit message says:

The renderer in cluster-kube-apiserver-operator will need to be also changed to stop creating the same manifest file.

But on a quick skim, I didn't see any PRs there making that change. Can you link us to the operator change?

Opened openshift/cluster-kube-apiserver-operator#97

@sanchezl sanchezl changed the title add kube-system-configmap-etcd-serving-ca.yaml add configmap etcd-serving-ca.kube-system Oct 31, 2018
@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2018
@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@abhinavdahiya
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@abhinavdahiya abhinavdahiya added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2018
tier: "control-plane"
k8s-app: "kube-apiserver"
openshift.io/control-plane: "true"
openshift.io/component: "api"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As here, it would be nice to know what these labels are for. That makes us less likely to break things accidentally if someone proposes we change/remove one of them later ;).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As here, it would be nice to know what these labels are for. That makes us less likely to break things accidentally if someone proposes we change/remove one of them later ;).

@sanchezl they aren't for us. Remove all labels and the etcd operator can sort out what they want to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed them all.

@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 62da5b2 to 81f5066 Compare November 1, 2018 04:57
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 1, 2018
@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 81f5066 to 414aab1 Compare November 1, 2018 14:01
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2018
@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 414aab1 to 67124a9 Compare November 1, 2018 14:38
@deads2k deads2k removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2018
@deads2k
Copy link
Contributor

deads2k commented Nov 2, 2018

@abhinavdahiya it was rebased and labels removed. As a general statement, do you mind me re-labeling simple changes like that?

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Nov 2, 2018

@abhinavdahiya it was rebased and labels removed. As a general statement, do you mind me re-labeling simple changes like that?

Sure, except approved and lgtm ;p :D
Usually the CI handles the rebase labels, but somehow here it forgot

/lgtm

@abhinavdahiya abhinavdahiya added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2018
@openshift-merge-robot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from 67124a9 to f68c64f Compare November 2, 2018 18:15
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2018
@deads2k
Copy link
Contributor

deads2k commented Nov 2, 2018

@sanchezl indicate what changed and why

@sanchezl
Copy link
Contributor Author

sanchezl commented Nov 2, 2018

@sanchezl indicate what changed and why

rebased and generated new dependency graph

Manifest to create this resources belongs in installer. The renderer in
cluster-kube-apiserver-operator will need to be also changed to stop
creating the same manifest file.

Also, updated the dependency graph at docs/design/resource_dep.svg
@sanchezl sanchezl force-pushed the move_manifest_etcd_serving_ca_configmap branch from f68c64f to 7294a3d Compare November 4, 2018 18:04
@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, sanchezl

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

@deads2k
Copy link
Contributor

deads2k commented Nov 5, 2018

/retest

just to see

@openshift-merge-robot openshift-merge-robot merged commit 336c87e into openshift:master Nov 5, 2018
@sanchezl sanchezl deleted the move_manifest_etcd_serving_ca_configmap branch March 19, 2019 18:14
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants