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

re-enable the content-check of validate #152

Merged
merged 1 commit into from May 7, 2020

Conversation

devfaz
Copy link
Contributor

@devfaz devfaz commented Jan 29, 2020

6 years ago the commit a26a8a1 disabled the content-check of validate.

The module just checked if a connection is possible, but doesnt check if f.e. WSREP is ready.
This commit fixes this by re-adding the grep-call

@fraenki fraenki self-assigned this Feb 9, 2020
@fraenki
Copy link
Member

fraenki commented Feb 9, 2020

for reference:
a26a8a1

@fraenki
Copy link
Member

fraenki commented Feb 9, 2020

@devfaz Thanks for your contribution! However, it looks like it was changed for a reason. Could you elaborate more on why you think it should be changed (again)? What would be improved? What are the risks?

On which database vendor + version has this been tested?

@devfaz
Copy link
Contributor Author

devfaz commented Feb 10, 2020

@fraenki according to the commit msg. This was done to "optimize" the code by using retry, sleep of puppet instead of bash-code.
Seems like a typical "i dont like this code, i optimize it - oh did test it well - now its broken"-workflow.

What would be improved? Well it works again? The code currently isnt working at all, because its no longer looking for "truecatch" and so it just checks if connection is possible (without checking if wsrep is ready)

This is not the latest version of my fix, so plz hold the line - I will update the commit asap.

@fraenki
Copy link
Member

fraenki commented Feb 10, 2020

This is not the latest version of my fix, so plz hold the line - I will update the commit asap.

Sure, waiting for your updated fix.

@fraenki fraenki added this to the 2.0.0 milestone Feb 11, 2020
@fraenki fraenki changed the title 6 years ago the commit a26a8a16d33519d9cc97e8200bc00795ae0560b1 disab… re-enable the content-check of validate Feb 17, 2020
@fraenki
Copy link
Member

fraenki commented Feb 18, 2020

@devfaz I'm currently preparing version 2.0.0 of this module. I think this change would suit well in this major release, because it already contains several breaking changes.

@fraenki fraenki removed this from the 2.0.0 milestone Apr 6, 2020
@fraenki
Copy link
Member

fraenki commented Apr 9, 2020

@devfaz I know the current situation is not ideal, but may I ask you to rebase your PR against the current master? Thanks!

devfaz pushed a commit to noris-network/puppet-galera that referenced this pull request Apr 15, 2020
@devfaz
Copy link
Contributor Author

devfaz commented Apr 15, 2020

@fraenki - I just rebased my change. In the meantime I found out, that my previous try to workaround the "ERROR"-detection wasnt working in every edge-case, but the topic of this issue is solved by this commit.

I will create another issue for the "ERR"-detection issue.

@fraenki fraenki added this to the 2.1.0 milestone May 7, 2020
@fraenki fraenki merged commit 50c0d7a into markt-de:master May 7, 2020
@fraenki
Copy link
Member

fraenki commented May 7, 2020

@devfaz Merged, thanks!

@fraenki
Copy link
Member

fraenki commented May 7, 2020

Released as 2.1.0.

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

Successfully merging this pull request may close these issues.

None yet

2 participants