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.

Sign up for GitHub

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

Jump to bottom

switch to using upstream patch cmd #20665

Merged
merged 2 commits into from
Aug 21, 2018
Merged

switch to using upstream patch cmd #20665

merged 2 commits into from
Aug 21, 2018

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Aug 15, 2018 •
edited
Loading

Work in progress...

Depends on #20642
Picks kubernetes/kubernetes#67399

Diff between old and new master config patch (hack/lib/start.sh) using changes from this PR: http://www.mergely.com/0HlByqZs/

cc @soltysh @deads2k

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 15, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch 7 times, most recently from 7ec1083 to c343357 Compare August 16, 2018 20:21
@juanvallejo
Copy link
Contributor Author

Seems to be failing when I redirect final output to file:
[ERROR] PID 22317: hack/lib/start.sh:171: oc patch --local --type=json -o yaml -f - --patch="[{\"op\": \"replace\", \"path\": \"/imagePolicyConfig/maxImagesBulkImportedPerRepository\", \"value\": ${MAX_IMAGES_BULK_IMPORTED_PER_REPOSITORY:-5}}]" > "${SERVER_CONFIG_DIR}/master/master-config.yaml" exited with status 1.

@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch from b4ff3c9 to c343357 Compare August 16, 2018 20:41
@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch from 9d2aba4 to c343357 Compare August 16, 2018 21:30
@juanvallejo
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch from abc4a42 to 4da9ee6 Compare August 17, 2018 15:14
@juanvallejo
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch 3 times, most recently from 2082554 to d51d369 Compare August 17, 2018 15:48
@juanvallejo juanvallejo mentioned this pull request Aug 17, 2018
Merged
@juanvallejo
Copy link
Contributor Author

/retest

deads2k
deads2k reviewed Aug 17, 2018
View reviewed changes
hack/lib/start.sh
# Make oc use ${MASTER_CONFIG_DIR}/admin.kubeconfig, and ignore anything in the running user's $HOME dir
export ADMIN_KUBECONFIG="${MASTER_CONFIG_DIR}/admin.kubeconfig"
CLUSTER_ADMIN_CONTEXT=$(oc config view --config="${ADMIN_KUBECONFIG}" --flatten -o template --template='{{index . "current-context"}}'); export CLUSTER_ADMIN_CONTEXT
${sudo} chmod -R a+rwX "${ADMIN_KUBECONFIG}"
os::log::debug "To debug: export KUBECONFIG=$ADMIN_KUBECONFIG"

cp "${SERVER_CONFIG_DIR}/master/master-config.yaml" "${SERVER_CONFIG_DIR}/master/master-config.orig.yaml"
oc patch --config="${ADMIN_KUBECONFIG}" --local --type=json -o yaml -f "${SERVER_CONFIG_DIR}/master/master-config.orig.yaml" --patch="[{\"op\": "replace", \"path\": \"/etcdConfig/address\", \"value\": \"${API_HOST}:${ETCD_PORT}\"}]" | \
Copy link
Contributor

Choose a reason for hiding this comment

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

none of these local calls should need a kubeconfig. Why do they seem to?

deads2k
deads2k reviewed Aug 17, 2018
View reviewed changes
cmd/oc/oc.go Outdated
@@ -7,7 +7,7 @@ import (
"runtime"
"time"

"k8s.io/apiserver/pkg/util/logs"
"k8s.io/kubernetes/pkg/api/legacyscheme"
Copy link
Contributor

@deads2k deads2k Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this file should not be touched.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this bit should not be touched, but we still need to install the configapi into kubectl's scheme, no?

@deads2k
Copy link
Contributor

deads2k commented Aug 17, 2018

looks like you need to update on top of my branch. I might squash the changes together. i wasn't expect a --config to be required.

@juanvallejo
Copy link
Contributor Author

@deads2k

looks like you need to update on top of my branch. I might squash the changes together. i wasn't expect a --config to be required.

Will rebase with your branch. A --config was needed in order to avoid updating the upstream patch command. It fails here without a kubeconfig.

@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch from d51d369 to 16d8b1a Compare August 20, 2018 14:12
@juanvallejo
Copy link
Contributor Author

@deads2k @soltysh this is now up to date with #20642 (latest master)

@juanvallejo juanvallejo force-pushed the jvallejo/switch-to-upstream-patch-cmd branch from 16d8b1a to b61a864 Compare August 20, 2018 14:13
@juanvallejo
Copy link
Contributor Author

@deads2k per our conversation, will continue using --config here while we fix the upstream patch command. I have opened an issue to track this here kubernetes/kubectl#523

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Aug 20, 2018 •
edited
Loading

@deads2k looks like I am no longer able to use the upstream patch command with Config types

cat master-config.yaml | oc patch --local --type=json -o yaml -f - --patch="[{\"op\": \"replace\", \"path\": \"/imagePolicyConfig/maxImagesBulkImportedPerRepository\", \"value\": ${MAX_IMAGES_BULK_IMPORTED_PER_REPOSITORY:-5}}]"
error: unable to recognize "STDIN": no matches for kind "MasterConfig" in version "v1"

@soltysh
Copy link
Contributor

soltysh commented Aug 21, 2018

@deads2k looks like I am no longer able to use the upstream patch command with Config types

You still need to install the config types, not sure why @deads2k removed that part from his PR.

@juanvallejo
Copy link
Contributor Author

/retest

@deads2k
Copy link
Contributor

deads2k commented Aug 21, 2018

@juanvallejo please write a release note about removing the experimental patch in a future release.
/lgtm
/hold

remove the hold once you have a release note written.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 21, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, juanvallejo

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018
@juanvallejo juanvallejo mentioned this pull request Aug 21, 2018
Closed
@juanvallejo
Copy link
Contributor Author

@deads2k Added release note: openshift/openshift-docs#10458 (comment)

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2018
@openshift-merge-robot openshift-merge-robot merged commit 7e70dc8 into openshift:master Aug 21, 2018
@juanvallejo juanvallejo deleted the jvallejo/switch-to-upstream-patch-cmd branch August 21, 2018 22:37
@tobybellwood tobybellwood mentioned this pull request Nov 20, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@knobunc knobunc Awaiting requested review from knobunc

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@deads2k deads2k

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@juanvallejo @deads2k @soltysh @openshift-ci-robot @openshift-merge-robot