-
Notifications
You must be signed in to change notification settings - Fork 7
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
Case insensitive admin search, testing updates and manager setting of notes and report status #281
Merged
Merged
Changes from 2 commits
Commits
Show all changes
60 commits
Select commit
Hold shift + click to select a range
3524e9e
Remove --runInBand
dcloud 60b68e0
Add some docs around testing
dcloud bb1a516
Merge branch 'main' into dcloud/test-failure-fixes
dcloud 8484d6e
Allow search to be case-insensitive
4ef5971
Fix Eslint issue
3f43234
Fix example
dcloud deac317
Rename docker-compose file used for dynamic security scans
dcloud f3f9022
Create bin/docker script, which will make command composition easier
dcloud 44af291
Remove --runInBand
dcloud 9b422f3
Add option parsing and rename docker script
dcloud 84b24b8
Multiple docker-compose configs for multiple environments
dcloud 92a41c4
bin/docker-run fixes, improvements
dcloud 32f38c8
Minor docker-run improvements
dcloud de13b1f
Change 'conf' to be a string, since that's how it gets used. Echo cmd…
dcloud a232a91
docker-run: 'test' uses test env by default
dcloud ac43405
Undo the composed compose files; docker-compose wasn't handling diffe…
dcloud 188b05f
Remove 2-liners in favor of echo_exec function
dcloud ece7bb4
docker-run: Make default config a readonly var
dcloud c8886f0
Add simpler run-tests script. Improve docker-compose.test.yml with na…
dcloud 8b0bece
Remove bin/docker-run since we are no longer composing multiple
dcloud 43e18c9
One last rename
dcloud 90c4e50
Managers can add notes and set status of reports
jasalisbury 975711e
Add API docs
jasalisbury fe19276
Update DB diagram
jasalisbury 58b12ac
Merge branch 'main' into dcloud/test-failure-fixes
dcloud 13ff30f
Simplify networking, remove unused containers per @dcmcand
dcloud e56cf18
update failing tests
43ed721
Merge branch 'main' into js-137-manager-approves-report
jasalisbury 486acd4
Add section on caveats with creating/deleting databas records
dcloud 6dee177
Merge branch 'main' into dcloud/test-failure-fixes
dcloud b14eeb9
Split run-test into multiple steps, per @dcmcand
dcloud 11b92b9
Fix API docs. Split review page into components. Add DECIMAL_BASE
jasalisbury f698be6
Merge branch 'js-137-manager-approves-report' of github.com:adhocteam…
jasalisbury 25e5f1f
Switch shortcut, also per @dcmcand
dcloud d227006
Merge branch 'main' into dcloud/split-docker-local-test
dcloud 0a785fd
Merge branch 'main' into 274-allow-case-insensitive-search
gopar 4eadc38
Merge branch 'main' into dcloud/test-failure-fixes
dcloud b3fb374
Merge branch 'main' into dcloud/test-failure-fixes
dcloud 9e05993
Merge pull request #134 from adhocteam/dcloud/test-failure-fixes
dcloud b80d852
Merge branch 'main' into dcloud/split-docker-local-test
dcloud e976f8a
Merge branch 'main' into 274-allow-case-insensitive-search
gopar db9cb58
Merge pull request #138 from adhocteam/dcloud/split-docker-local-test
dcloud 2956987
Merge branch 'main' into 274-allow-case-insensitive-search
gopar eb867ac
Merge pull request #137 from adhocteam/274-allow-case-insensitive-search
gopar 75e4df0
Merge branch 'main' into js-137-manager-approves-report
jasalisbury d31f14f
Merge pull request #139 from adhocteam/js-137-manager-approves-report
jasalisbury a50285f
Increase wait time for the server to spin up in cucummber ci tests
jasalisbury ba18d84
Merge pull request #145 from adhocteam/js-increase-ci-cucumber-wait-time
jasalisbury d29a75d
Document bin/run-tests, make sure frontend tests run in frontend inst…
dcloud 3f47788
Feedback from PR
jasalisbury c99ba9a
Add frontend tests
jasalisbury e027cdf
Remove unused import
jasalisbury c6f62d6
Merge pull request #147 from adhocteam/dcloud/docker-tests-changes-pe…
dcloud 4680a12
Merge branch 'main' into js-manager-notes-feedback
jasalisbury 5a31b94
Merge pull request #148 from adhocteam/js-manager-notes-feedback
jasalisbury c08eb4c
Somehow service == container_name for docker-compose exec
dcloud 6cf1025
Add param to run-tests to choose to run only frontend or backend test…
dcloud c4c7bd4
Check exit codes of docker cmds, and 'log' errors. Return exit_code e…
dcloud 68a76de
Merge branch 'main' into dcloud/test-script-improvements
dcloud f327246
Merge pull request #149 from adhocteam/dcloud/test-script-improvements
dcloud File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. Looks like a missing
container_name
for the frontend service.--cwd frontend
is also unnecessary (though doesn't break anything) in the frontend container.That also exposes that this script papers over any failures if you don't read the output carefully. Since this is not used in CI/CD I think I'm ok with that, but I think it'd be good to collect exit statuses and exit the script with non-zero if any individual step exits with non-zero, or just bail out as soon as we get a non-zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I've run this a few times and haven't seen this problem. I'll take a look again. Could you get me the output of
docker-compose -f docker-compose.test.yml ps --all
on your machine?As for handing failures, it was intentional that we didn't error out if one of the intermediate steps threw an error. We could catch those statuses, but I'm not sure we want to prevent frontend tests from running if the backend tests fail, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I went back and forth on that. I'm 100% fine with all the steps running as long as I get a nice clear indication that at least one step failed when the script exits.
Here's the
ps
output:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick check of the doc page for
docker-compose exec
, it sounds like that command takes a service name rather than a container name, so that might be related.While we're discussing, I had originally considered having the frontend and backend tests run as separate scripts, since it seems like engineers might be focused on either frontend or backend during development. Would you be interested in making this accept an optional frontend/backend param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's quick, sure, but that's pretty low priority to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was quick: https://github.com/adhocteam/Head-Start-TTADP/pull/149/files
I also added some logging of errors and have the script return an exit code value equal to the number of script failures.