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

Revert polycubed persistency support #160

Merged
merged 1 commit into from
Jun 28, 2019
Merged

Revert polycubed persistency support #160

merged 1 commit into from
Jun 28, 2019

Conversation

mauriciovasquezbernal
Copy link
Contributor

@mauriciovasquezbernal mauriciovasquezbernal commented Jun 27, 2019

We found some bugs that make polycube unusable at this point. So, revert this
until we have a more robust solution.

There is not a clear idea yet of what are the bugs, but many tests started to fail after this was merged:
http://130.192.225.104:8080/blue/organizations/jenkins/polycube-test/detail/master/94/pipeline/

We found some bugs that make polycube unusable at this point.  So, revert this
until we have a more robust solution.

This reverts:
- 69d888d ("add topology load at startup and save to memory and file after each modification")
- f47d3bc ("add persistency documentation")
- 2b42e45 ("Rephrasing documentation.")

Partially reverts #141.

Signed-off-by: Mauricio Vasquez B <mauriciovasquezbernal@gmail.com>
@mauriciovasquezbernal
Copy link
Contributor Author

Almost all tests are passing[1], there are some problems with the shadow router but it should be another issue, my suggestion is to merge this now to make polycube usable again.

[1] http://130.192.225.104:8080/blue/organizations/jenkins/polycube-test/detail/PR-160/1/pipeline/

@mauriciovasquezbernal
Copy link
Contributor Author

It appears @richiMarchi already has a solution for it, waiting for his PR then.

@acloudiator
Copy link
Contributor

There is not a clear idea yet of what are the bugs, but many tests started to fail after this was merged:

@mauriciovasquezbernal @mbertrone AFAIK, the merge to master for any PR is blocked if the docker build is failed, isn't it? What would be the case here in PR#141?!

@mauriciovasquezbernal
Copy link
Contributor Author

There is not a clear idea yet of what are the bugs, but many tests started to fail after this was merged:

@mauriciovasquezbernal @mbertrone AFAIK, the merge to master for any PR is blocked if the docker build is failed, isn't it? What would be the case here in PR#141?!

The merge is not blocked by the docker build, additional this build does not perform the tests, those are done with jenkins.

So far this is manual procedure, one before merging should check is tests are ok.

@acloudiator
Copy link
Contributor

So far this is manual procedure, one before merging should check is tests are ok.

Exactly you made my point, the merge was done without the test. Not getting into details, but certainly, it's a lesson learned @polycube-network/pcn-maintainers

Also, we might want to think of automating this procedure, rather than manual.

@mauriciovasquezbernal
Copy link
Contributor Author

The fix for the problem appears not to be that trivial, so I suggest to merge this PR in order to fix polycube, once the fix is ready we can reintroduce the whole feature.

@acloudiator
Copy link
Contributor

The fix for the problem appears not to be that trivial, so I suggest to merge this PR in order to fix polycube, once the fix is ready we can reintroduce the whole feature.

@frisso If the change isn’t that straightforward then I agree with @mauriciovasquezbernal .
Would you please approve and merge, if you too agree on this.

@frisso frisso merged commit ff6e368 into master Jun 28, 2019
@mauriciovasquezbernal mauriciovasquezbernal deleted the pr/revert_pr_141 branch June 28, 2019 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants