Try Make Loading Profile Backward Compatible #7578
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
|
Welcome @eashi! |
k8s-ci-robot
added
cncf-cla: no
|
Hi @eashi. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XL
|
Can one of the admins verify this patch? |
|
this is a very promising PR ! thank you for doing it. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
priyawadhwa
reviewed
May 29, 2020
|
@eashi are you sitll working on this? |
|
Hey @medyagh sorry I was side tracked by a presentation I have to do on this Thursday. I will jump back to this right after the presentation starting Friday. If this is a priority I am happy to collaborate on this with anyone who wants to take over :). But hopefully I will be able to jump to this again this Friday. |
Travis tests have failedHey @eashi, 1st Buildmake test2nd Buildmake testTravisBuddy Request Identifier: 5c217ed0-ad1c-11ea-8245-836d9da6cb4c |
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
Travis tests have failedHey @eashi, 1st Buildmake test2nd Buildmake testTravisBuddy Request Identifier: b03ce2a0-ad5f-11ea-8b2d-cffda33d3704 |
Travis tests have failedHey @eashi, 1st Buildmake test2nd Buildmake testTravisBuddy Request Identifier: 8d217de0-ae3a-11ea-b5ce-3bd953fd8c80 |
Travis tests have failedHey @eashi, 1st Buildmake testTravisBuddy Request Identifier: 416859b0-b235-11ea-b4e4-a33918451e6b |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eashi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #7578 +/- ##
==========================================
+ Coverage 33.87% 34.04% +0.16%
==========================================
Files 154 155 +1
Lines 9897 9955 +58
==========================================
+ Hits 3353 3389 +36
- Misses 6140 6160 +20
- Partials 404 406 +2
|
Thanks to tkoster
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
I signed it |
k8s-ci-robot
added
cncf-cla: yes
|
@medyagh @priyawadhwa This PR is very close to graduated to a non-draft PR. The only remaining thing I can see is a unit test for version 1.5.2 configuration. Please let me know if you have any comments, and let me know how many versions back you want this backward-compatibility work. |
|
Ok, I have added the unit tests for version 1.5.2. Things look ok from my side. |
priyawadhwa
reviewed
Jul 16, 2020
There was a problem hiding this comment.
Hey @eashi sorry for the delayed response! I'll run the integration tests and we can see where we're at.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
kvm2 Driver Times for Minikube (PR 7578): [64.956389996 65.004226899 64.706588809] Averages Time Per Log docker Driver Times for Minikube (PR 7578): [25.110516317 26.590671067 25.728241016] Averages Time Per Log |
|
We ended up fixing this issue in v1.12.x by fixing the individual changes. We now test upgrades from v1.0 and onward as part of our integration tests. This was a valiant PR, but I'm going to close this until we are able to identify the need for the additional code. My apologies for letting this one sit for so long :( |
|
@tstromberg thanks for taking the time to close this one properly. Glad things are going the right direction regardless of solution, was fun trying a stab on it anyway. Cheers :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7062
Note: This might be a little too big PR, but it doesn't have to be; we can trim it down. I am happy to move it to "enhancements" if you like.
Without this PR, when developers introduce changes to profile configurations it may break old profiles. Minikube would try to load the profile and fails, then it prompts the user to delete those profiles than suggest to fix them.
I wrote about this in my blog here
With this PR, the code will try to load the profile, if it fails then it will try to load the profiles using the previous versions of the profile configuration using the old code, and then tries to translate it to the new configuration.
E.g. Imagine that the user has old profiles on version v1.5.2, and then she installed v1.9.2. The code will try to load the profiles using the v1.9.2 code, if it fails it will try using the code v1.8.2 if it fails it will try using code v1.7.2. If it succeeds it will load the profile and then tries to translate it to v1.8.2, if it succeeds then it will translate it to v1.9.2 and bubbles up to the current version.
This is done recursively through the method
tryTranslatein thetranslators.go. The method will recurse on the arrayversionConfigTranslatorswhich will hold translators for older configurations.I wanted to load the old profiles as close as they used to be loaded, so we will create a folder for each version in which the profile config has changed, and put the old code (if possible) in there with the least amount of change. This, however, is taking the toll of this PR, I hope there is an easier way, or that we can be very selective in what we take from the old code.
The PR is far from being ready, but the main skeleton is there. Any help or suggestions is highly welcomed.