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

simplify logic to get the proxy configmap #288

Merged
merged 3 commits into from Jan 15, 2020
Merged

simplify logic to get the proxy configmap #288

merged 3 commits into from Jan 15, 2020

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 3, 2020

There are already well established controllers to maintain the content of proxy configmaps. This PR removes non-standard duplication of this logic. Auto-rollouts are still driven based on the hashes contained in the config configmap.

/assign @stlaz

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 3, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2020
@stlaz
Copy link
Member

stlaz commented Jan 6, 2020

/hold
this causes redeployment loop since the CM in question is being injected by the network operator

@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 Jan 6, 2020
@deads2k
Copy link
Contributor Author

deads2k commented Jan 6, 2020

/hold
this causes redeployment loop since the CM in question is being injected by the network operator

That loop already logically exists, you just hid it behind more complex logic. Since the network operator doesn't require the openshift-apiserver-operator, no logical cycle exists.

@stlaz
Copy link
Member

stlaz commented Jan 6, 2020

@deads2k What I meant is that there's a logical loop where you create the CM, it gets injected with CAs by the network operator, then you remove the data by reapplying the CM with empty data.

This was the very reason why I did not use the logic from this PR when I did the CM sync originally.

See tail of https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-openshift-apiserver-operator/288/pull-ci-openshift-cluster-openshift-apiserver-operator-master-e2e-aws/1672/artifacts/e2e-aws/pods/openshift-apiserver-operator_openshift-apiserver-operator-55f5c4dc84-8lf9g_openshift-apiserver-operator.log

@deads2k
Copy link
Contributor Author

deads2k commented Jan 6, 2020

@deads2k What I meant is that there's a logical loop where you create the CM, it gets injected with CAs by the network operator, then you remove the data by reapplying the CM with empty data.

This was the very reason why I did not use the logic from this PR when I did the CM sync originally.

So I see. I guess at some point we decided to stomp and not merge. Guess I'll add a merge directive

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2020
@deads2k deads2k force-pushed the remove-unnecessary-custom-logic branch from 4ebbe17 to 1dca4c9 Compare January 6, 2020 21:15
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2020
@deads2k
Copy link
Contributor Author

deads2k commented Jan 6, 2020

/hold

needs upstream

@stlaz see if this fixes your concern.

@stlaz
Copy link
Member

stlaz commented Jan 7, 2020

@deads2k it's a hack which special-cases a single type of CM, on the other hand I can't think of anything better at the moment, I guess we'll have to go with it for the time being.

@deads2k deads2k force-pushed the remove-unnecessary-custom-logic branch from 1dca4c9 to ef16eb9 Compare January 10, 2020 17:43
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2020
@deads2k deads2k force-pushed the remove-unnecessary-custom-logic branch from ef16eb9 to 38d2a90 Compare January 10, 2020 17:44
@deads2k deads2k removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@deads2k deads2k force-pushed the remove-unnecessary-custom-logic branch from 38d2a90 to 83d9214 Compare January 13, 2020 14:00
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020
@openshift-bot
Copy link
Contributor

/retest

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

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

10 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@p0lyn0mial
Copy link
Contributor

e2e-aws-operator will be fixed by #297

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@deads2k deads2k force-pushed the remove-unnecessary-custom-logic branch from 83d9214 to ba6b40a Compare January 14, 2020 15:47
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2020
@p0lyn0mial
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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-bot
Copy link
Contributor

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit e8437d3 into openshift:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh Awaiting requested review from soltysh

@sttts sttts Awaiting requested review from sttts

Assignees

@stlaz stlaz

@p0lyn0mial p0lyn0mial

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@deads2k @stlaz @openshift-bot @p0lyn0mial @openshift-ci-robot @openshift-merge-robot