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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix/fix lint #2577

Merged
merged 2 commits into from May 17, 2017
Merged

Hotfix/fix lint #2577

merged 2 commits into from May 17, 2017

Conversation

brylie
Copy link
Contributor

@brylie brylie commented May 17, 2017

Don't hit me 馃槰

@marla-singer
Copy link
Contributor

marla-singer commented May 17, 2017

@brylie It doesn't work. Now if list contains proxy backend with non-existent API then there is an empty place
joxi_screenshot_1495031201568

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

We need to clean up that proxy backend. Please refer to the discussion in the other PR, where we tried doing due diligence and decided to go with the basic fix.

This PR is a lint fix.

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

Well, it does work, but there may be corrupted data on the server. How did you test?

@marla-singer
Copy link
Contributor

marla-singer commented May 17, 2017

How did you test?

@brylie Create Proxy Backend with non-existent API ID. The same case as apinf.io has

@mauriciovieira
Copy link
Contributor

@marla-singer The logic is unaltered. So this empty name should happen also before this hotfix.

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

Create Proxy Backend with non-existent API ID. The same case as apinf.io has

Steps to reproduce?

Remember, there were three of us discussing this issue. We found out there were migration problems. I tested the cascading delete successfully.

It might just be a botched migration that caused inconsistent data, but the patch will prevent the original issue of the Dashboard being blank.

@marla-singer
Copy link
Contributor

@mauriciovieira I missed this bug in this PR #2540

}

return apiName;
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, @marla-singer, before this return here, it was returning undefined anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should this code block look like @mauriciovieira?

@marla-singer
Copy link
Contributor

@brylie But now apinf.io contains Proxy Backend document with non-existent API ID. And because of that Dashboard page is empty

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

In the case of the non-existant API, the data needs to be cleaned up. This hotfix did not address that issue, by design.

We can continue the work in a different pull request, but will need steps to reproduce the data inconsistency other than manually creating a proxy backend with malformed API ID.

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

But now apinf.io contains Proxy Backend document with non-existent API ID. And because of that Dashboard page is empty

The whole dashboard is empty? We haven't deployed this hotfix there yet, have we?

@mauriciovieira
Copy link
Contributor

Ah, I see https://github.com/apinf/platform/pull/2540/commits was not checked by travis.

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

Jep, and I wasn't reading my lint messages closely. My bad.

@marla-singer
Copy link
Contributor

@mauriciovieira What do you think? This PR fixes ESlint error but it seems that found bug can repeat on apinf.io
joxi_screenshot_1495031201568

@mauriciovieira
Copy link
Contributor

@brylie, @marla-singer I think that if the problem of #2528 is still there, even after #2540, we must not amend things. We must revert c3cb04a

@mauriciovieira
Copy link
Contributor

But if #2540 really closes #2528, this hotfix is appropriate here.

@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

I think that if the problem of #2528 is still there

Based on what? How can we reproduce?

Our strongest hypothesis so far is that it was a failed migration, please review the comments:

@mauriciovieira
Copy link
Contributor

@brylie I see your point. @marla-singer I agree with @brylie.

@marla-singer marla-singer merged commit 0042b2a into master May 17, 2017
@brylie
Copy link
Contributor Author

brylie commented May 17, 2017

Thank you @mauriciovieira. We can definitely continue researching this issue, but it is just not clear to me what to do differently.

@marla-singer thank you very much for being so thorough with your reviews :-)

@mauriciovieira
Copy link
Contributor

The problem we face is that we have no clear answer as to how the data became inconsistent

I wonder if it's related to migration changing #2431 (comment)

@mauriciovieira mauriciovieira deleted the hotfix/fix-lint branch May 17, 2017 16:45
@mauriciovieira
Copy link
Contributor

I did merge master back to develop 1eaa1ed after this hotfix was applied to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants