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

API-1835: separate apply-configuration-live from apply-configuration #718

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Oct 18, 2024

This does the initial split of live functions (things that must reach out to external services and may or may not work on hypershift due to networking configuration), from things that don't have that problem.

Builds on top of some library-go and multi-operator-manager PRs that need to land.

Comment on lines +235 to +239
// TODO MOM this call here is the one that fails. Using the return value of ApplyDeployment to make decisions doesn't work in a MOM world.
// TODO MOM workload.NewController needs to (somehow) use the data read *after* this write, without using data *in* this write.
// TODO MOM this may be possible by storing a hash of the desired content. If the hash of the desired content matches
// TODO MOM then there is no new deployment required. This would not prevent rapid writes to fields we aren't controlling.
// TODO MOM perhaps we actually need to find a way to request MOM inject a generation after a write? It is an operator standard.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is sticky. The hash probably won't work. Making MOM aware of writing workload generation may be the only way to achieve parity.

@bertinatto

@openshift-ci openshift-ci bot requested review from ibihim and liouk October 18, 2024 16:57
Copy link
Contributor

openshift-ci bot commented Oct 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2024
@deads2k deads2k changed the title separate apply-configuration-live from apply-configuration API-1835: separate apply-configuration-live from apply-configuration Oct 21, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 21, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 21, 2024

@deads2k: This pull request references API-1835 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to this:

This does the initial split of live functions (things that must reach out to external services and may or may not work on hypershift due to networking configuration), from things that don't have that problem.

Builds on top of some library-go and multi-operator-manager PRs that need to land.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2024
Copy link
Contributor

openshift-ci bot commented Oct 23, 2024

@deads2k: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agnostic-ipv6 252822f link false /test e2e-agnostic-ipv6
ci/prow/e2e-operator 252822f link true /test e2e-operator

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2024
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants