Skip to content

*: add resource sync for etcd-quorum-guard #623

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
Closed
hexfusion wants to merge 1 commit into from
Closed

*: add resource sync for etcd-quorum-guard #623

hexfusion wants to merge 1 commit into from

Conversation

@hexfusion
Copy link
Contributor

@hexfusion hexfusion commented Apr 10, 2019 •
edited
Loading

This PR adds functionality which will sync ConfigMaps and Secrets from openshift-config namespace to kube-system. The result will be etcd TLS resources available for etcd-quorum-guard` to consume.

/cc @abhinavdahiya

functionality assists #613

@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 Apr 10, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 10, 2019
@hexfusion hexfusion mentioned this pull request Apr 10, 2019
Closed
@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 10, 2019

This seems to work as expected and will simplify #613

@hexfusion hexfusion changed the title WIP: add resource sync *: add resource sync for proxy guard Apr 10, 2019
@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 Apr 10, 2019
@hexfusion hexfusion changed the title *: add resource sync for proxy guard *: add resource sync for etcd-quorum-guard Apr 10, 2019
hexfusion
hexfusion commented Apr 10, 2019
View reviewed changes
@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Apr 10, 2019

so this will be merged before #613 @hexfusion ?

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 10, 2019

so this will be merged before #613 @hexfusion ?

Yes it will need to merge before #613

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 10, 2019

2019/04/10 19:10:29 Running pod e2e-aws-op
2019/04/10 19:11:45 error: unable to signal to artifacts container to terminate in pod e2e-aws-op, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2019/04/10 19:11:45 error: unable to retrieve artifacts from pod e2e-aws-op: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")
2019/04/10 19:11:50 Ran for 4m31s
error: could not run steps: template pod "e2e-aws-op" failed: pod e2e-aws-op was already deleted

/test e2e-aws-op

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Apr 10, 2019

thanks @hexfusion added a note onto the other PR. 👍

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 10, 2019

/test e2e-aws-op

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Apr 10, 2019

HAProxy issues:
/test e2e-aws

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Apr 10, 2019

aws VPC issues, been hitting these all day. I dont know that retesting is going to work right now.

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 10, 2019

/test e2e-aws

/joke

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 10, 2019

@hexfusion: Sore throats are a pain in the neck!

Details

In response to this:

/test e2e-aws

/joke

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.

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Apr 10, 2019

Not again... 😿

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 11, 2019

/test e2e-aws

1 similar comment
@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 11, 2019

/test e2e-aws

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 11, 2019

@runcom PTAL

@runcom
Copy link
Member

runcom commented Apr 12, 2019

Looks good to me, I would love @abhinavdahiya to take a look at this for merging tho

/approve

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hexfusion, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Apr 12, 2019
@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 16, 2019

@abhinavdahiya PTAL

abhinavdahiya
abhinavdahiya reviewed Apr 17, 2019
View reviewed changes
lib/resourceapply/core.go Outdated
Copy link
Contributor

@abhinavdahiya abhinavdahiya Apr 17, 2019

Choose a reason for hiding this comment

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

deepcopy just be safe.

Copy link
Contributor Author

@hexfusion hexfusion Apr 18, 2019

Choose a reason for hiding this comment

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

ack

abhinavdahiya
abhinavdahiya reviewed Apr 17, 2019
View reviewed changes
lib/resourceapply/core.go Outdated
Copy link
Contributor

@abhinavdahiya abhinavdahiya Apr 17, 2019

Choose a reason for hiding this comment

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

same as https://github.com/openshift/machine-config-operator/pull/623/files#r276461924

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Apr 17, 2019

LGTM

@runcom
Copy link
Member

runcom commented Apr 18, 2019

Perhaps it would make sense to combine this with #613 so I can test the whole wad together.

YES 😄

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 18, 2019

@RobertKrawitz go for it if you add my fork as a remote you can grab the commit from add_sync branch

@RobertKrawitz
Copy link
Contributor

RobertKrawitz commented Apr 18, 2019

I simply patched this in. I had to make a few other changes (to my code) for compatibility.

RobertKrawitz added a commit to RobertKrawitz/machine-config-operator that referenced this pull request Apr 23, 2019
@RobertKrawitz
Apply openshift#623
ce3acd6
@runcom
Copy link
Member

runcom commented Apr 24, 2019

this is in #613 can we close?

@RobertKrawitz
Copy link
Contributor

RobertKrawitz commented Apr 24, 2019

Yes, go ahead and close it.

@runcom runcom closed this Apr 24, 2019
@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 25, 2019

This work continues on #665 for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hexfusion @kikisdeliveryservice @openshift-ci-robot @runcom @abhinavdahiya @RobertKrawitz