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

Disable ensureSameOrigin #4000

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Disable ensureSameOrigin #4000

merged 5 commits into from
Jul 13, 2023

Conversation

johnbelamaric
Copy link
Contributor

During our testing in Nephio, we have found that ensureSameOrigin seems to erroneously flag updates as invalid, so I think we should disable it for now.

@johnbelamaric johnbelamaric requested a review from a team as a code owner July 7, 2023 19:09
@mortent
Copy link
Contributor

mortent commented Jul 7, 2023

Do we know exactly what was causing the problem? I think the ensureSameOrigin logic had some potential problems and I'm less convinced about the usefulness of this than when we introduced it, but it would be helpful to understand exactly what happened.

@johnbelamaric
Copy link
Contributor Author

No, not yet. I did add some logging; it showed that in the cases we're talking about, it fell through to the very last return statement.

I can try to capture a specific package that fails. Maybe we can fix it. I disabled it in the Nephio build to work around the problem, but I can spend a little more time on it now.

@johnbelamaric
Copy link
Contributor Author

I added another package this happens with in #3980. But actually it seems to be happening with almost every package created via clone. If I clone it, I can't do a copy. I am wondering if we have an e2e test that does a clone-and-copy?

I think we probably want to merge this PR while we figure it out.

@johnbelamaric johnbelamaric mentioned this pull request Jul 7, 2023
Closed
@johnbelamaric
Copy link
Contributor Author

Ok, based on #3980 (comment) I have removed the ensureSameOrigin altogether, and flipped the default for replay strategy.

@johnbelamaric
Copy link
Contributor Author

Fixes #3980

@johnbelamaric
Copy link
Contributor Author

rebased

@mortent
Copy link
Contributor

mortent commented Jul 10, 2023

This looks good to me. I also want to see if @natasha41575 has any input on this before we merge it.

@johnbelamaric
Copy link
Contributor Author

May be a legit failure do to replay strategy flip, let me take a closer look.

@johnbelamaric
Copy link
Contributor Author

Ok, yeah, fixed the legit failure and also fixed a flake in the PVS unit tests.

@mortent
Copy link
Contributor

mortent commented Jul 13, 2023

I think we've given enough time for feedback on this one, so I think we are good to merge it.

@johnbelamaric johnbelamaric merged commit 5ad4116 into kptdev:main Jul 13, 2023
16 checks passed
johnbelamaric added a commit to mortent/kpt that referenced this pull request Sep 18, 2023
@johnbelamaric
Disable ensureSameOrigin (kptdev#4000)
173641b 
* Disable ensureSameOrigin

* Default replay-strategy to false; remove ensureSameOrigin

* Remove same origin tests too

* Specify replay-strategy for this copy call

* Sort the created list to avoid flakes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortent mortent mortent approved these changes

@justinsb justinsb Awaiting requested review from justinsb justinsb was automatically assigned from GoogleContainerTools/kpt-reviewers

@natasha41575 natasha41575 Awaiting requested review from natasha41575

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johnbelamaric @mortent