spiffe / spiffe-helper

The SPIFFE Helper is a tool that can be used to retrieve and manage SVIDs on behalf of a workload
Apache License 2.0
43 stars 40 forks source link

Adds integration tests for mysql, postgres and go client-server. #72

Closed FedeNQ closed 1 year ago

FedeNQ commented 1 year ago

Changes proposal:

faisal-memon commented 1 year ago

Thanks @FedeNQ for writing all these unit tests.

faisal-memon commented 1 year ago

Can you move the tests form it/ to .github/tests?

FedeNQ commented 1 year ago

Hi! Thanks for the review, we've made some changes based on the comments that you've provided.

Can you move the tests form it/ to .github/tests?

In order to add the build-matrix we've created a bunch of scripts on .github/tests that call the required scripts with parameters, should we move ALL the scripts to .github/tests or is it correct as it is now?

faisal-memon commented 1 year ago

Hi @FedeNQ

Thanks for making the requested changes.

In order to add the build-matrix we've created a bunch of scripts on .github/tests that call the required scripts with parameters, should we move ALL the scripts to .github/tests or is it correct as it is now

Yes. Having the tests there would eliminate the need for those extra scripts and you could just call the tests directly.

Also if you can add a strategy: section that would be great. Then you won't need the for loop and can just do bash .github/tests/${{ matrix.values }}.

Described here: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

faisal-memon commented 1 year ago

Thanks @FedeNQ . The test matrix changes look good. The restore-entry.test.sh is failing and change-entry-test.sh is getting cancelled.

FedeNQ commented 1 year ago

Thanks @FedeNQ . The test matrix changes look good. The restore-entry.test.sh is failing and change-entry-test.sh is getting cancelled.

Thanks! Yes, it appears that there's an issue with building the container. I'm currently working on fix that problem.

FedeNQ commented 1 year ago

@faisal-memon It's seems like we had a false positive on some test, and a problem with the retries, but it should work now. Let me know if you see another change needed! Thanks

faisal-memon commented 1 year ago

Thanks @FedeNQ Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

FedeNQ commented 1 year ago

Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

The scripts on test folder call the scripts on IT folder with parameters, that's why they are necessary, so move them to .github/tests won't solve the problem (it will avoid only the code block used to move from test folder to IT folder) A solution could be move all to test folder and change the way we've made the scripts, having one test per case, instead of the parametizable tests that we've, should we?

faisal-memon commented 1 year ago

Can you move everything from it folder to .github/tests? That way you won't need those stub scripts.

The scripts on test folder call the scripts on IT folder with parameters, that's why they are necessary, so move them to .github/tests won't solve the problem (it will avoid only the code block used to move from test folder to IT folder) A solution could be move all to test folder and change the way we've made the scripts, having one test per case, instead of the parametizable tests that we've, should we?

No need to rewrite the tests. Just move to .github/tests then we can merge.

FedeNQ commented 1 year ago

I've moved the IT folder to .github Is it ok now, or should I move it to .github/tests?

faisal-memon commented 1 year ago

.github/tests since it is tests

faisal-memon commented 1 year ago

@FedeNQ I just noticed integration-tests (change-entry-test.sh) is failing

FedeNQ commented 1 year ago

@faisal-memon I made some changes, and now it passes all checks. Would it be a good idea to re-run all jobs to double-check?

faisal-memon commented 1 year ago

@faisal-memon I made some changes, and now it passes all checks. Would it be a good idea to re-run all jobs to double-check?

Thanks, looks good now.