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

reset isreported to MS_FALSE in msResetErrorList() (#6543) #6577

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

landryb
Copy link
Contributor

@landryb landryb commented Aug 3, 2022

No description provided.

@landryb
Copy link
Contributor Author

landryb commented Aug 3, 2022

fixes #6543 for me but more dragons might be lurking.

@jmckenna
Copy link
Member

jmckenna commented Aug 3, 2022

@landryb please also add a test for this in msautotest (follow guidelines at https://mapserver.org/development/tests/autotest.html ) and include a test in each of your pull requests. Thanks! (the more time you spend creating tests in msautotest, the less dragons faced in the future, ha)

@geographika
Copy link
Member

geographika commented Aug 3, 2022

Good catch @landryb! Resetting all fields makes sense to me.

I'm not sure this can be tested using msautotest, as it is the FastCGI that would need to be tested.
Even added an Apache test for this in #6294
Maybe similar could be done for spawn_fcgi?

@sdlime
Copy link
Member

sdlime commented Aug 3, 2022

Good catch @landryb! Resetting all fields makes sense to me.

I'm not sure this can be tested using msautotest, as it is the FastCGI that would need to be tested. Even added an Apache test for this in #6294 Maybe similar could be done for spawn_fcgi?

I can confirm this change fixes the issue on my installation. So this wasn't just an 8.0 issue... Nice @landryb!

@jmckenna jmckenna added the backport branch-8-0 To backport a pull request to branch-8-0 label Aug 3, 2022
@jmckenna jmckenna added this to the 8.0 Release milestone Aug 3, 2022
@jmckenna
Copy link
Member

jmckenna commented Aug 3, 2022

@landryb if you now fetch the latest commits from main into this PR branch, the build should run fine.

@landryb
Copy link
Contributor Author

landryb commented Aug 4, 2022

@landryb please also add a test for this in msautotest (follow guidelines at https://mapserver.org/development/tests/autotest.html ) and include a test in each of your pull requests. Thanks! (the more time you spend creating tests in msautotest, the less dragons faced in the future, ha)

e8ee9b8 should test that but with the existing apache setup. I dont have a linux setup around right now to go through all the nginx/spawn-fcgi setup madness :) Oh, and completely untested..

@landryb
Copy link
Contributor Author

landryb commented Aug 4, 2022

well afaict from the build job log the two tests are run:

2022-08-04T05:15:16.3933693Z Running CGI query
2022-08-04T05:15:16.4849104Z Running FastCGI query
2022-08-04T05:15:16.5642633Z Running FastCGI query again
2022-08-04T05:15:16.5756039Z Check that we return an error if given no arguments
2022-08-04T05:15:16.5850926Z Check again to make sure further errors are reported (#6543)

@jmckenna
Copy link
Member

jmckenna commented Aug 4, 2022

e8ee9b8 should test that but with the existing apache setup. I dont have a linux setup around right now to go through all the nginx/spawn-fcgi setup madness :) Oh, and completely untested..

thanks for adding this, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport branch-8-0 To backport a pull request to branch-8-0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants