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

Add tomcat 7 tests #4600

Merged
merged 4 commits into from Mar 8, 2019

Conversation

Projects
None yet
3 participants
@ProTip
Copy link
Contributor

ProTip commented Mar 7, 2019

Adds tomcat 7 API tests and updates Rundeck to achieve passing results.

@ProTip

This comment has been minimized.

Copy link
Contributor Author

ProTip commented Mar 7, 2019

Opened the PR early so we can all see the CI/CD status.

@ProTip ProTip requested review from sjrd218 and gschueler Mar 8, 2019

@ProTip

This comment has been minimized.

Copy link
Contributor Author

ProTip commented Mar 8, 2019

All tests passing, ready for review!

@ProTip

This comment has been minimized.

Copy link
Contributor Author

ProTip commented Mar 8, 2019

Saw wrong Travis notification and jumped the gun.. Looks like one of the tests is unstable ATM and that close() and flush() also need to be wrapped in a static compiled method.

@sjrd218

sjrd218 approved these changes Mar 8, 2019

Copy link
Contributor

sjrd218 left a comment

Overall it looks fine. I think we should probably remove all close() references and let grails/container close the output stream. The general rule for streams in Java is if you didn't create it you don't need to close it. If we close the output stream and some other thing attempts to write after the close is called then an IllegalStateException will be thrown. Since grails uses Interceptor classes which can do things after the controller returns I would say we probably shouldn't close the response output stream in the controller.

ProTip added some commits Mar 8, 2019

Replace close with flush to commit response
Move append and flush to a static method on base to DRY things up
@ProTip

This comment has been minimized.

Copy link
Contributor Author

ProTip commented Mar 8, 2019

@sjrd218 @gschueler All API tests are now passing on Tomcat 7.

I have removed close and replaced with flush which has the same response commit effect and stops Grails from trying to find a matching view; small change to keep similar behavour. I placed a static append and flush on the base controller to DRY things up.

@ProTip ProTip added this to the 3.0.17 milestone Mar 8, 2019

@gschueler gschueler merged commit 341f5d1 into master Mar 8, 2019

19 of 20 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - build.gradle (rundeck) No manifest changes detected
security/snyk - core/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/copyfile-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/flow-control-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/git-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/jasypt-encryption-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/job-state-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/localexec-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/orchestrator-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/script-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/source-refresh-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/stub-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - plugins/upvar-plugin/build.gradle (rundeck) No manifest changes detected
security/snyk - rundeck-storage/build.gradle (rundeck) No manifest changes detected
security/snyk - rundeckapp/build.gradle (rundeck) No manifest changes detected
security/snyk - rundeckapp/grails-spa/package.json (rundeck) No manifest changes detected
security/snyk - rundeckapp/metricsweb/build.gradle (rundeck) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.