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

fix: Issue with sql pool #181

Merged

Conversation

tuncaytunc-zf
Copy link
Contributor

Fixes the issue with sql pool by adding the missing dependencies.

Fixes the issue: #180

@florianrusch-zf
Copy link
Contributor

@tuncaytunc-zf do you know, why this wasn't catched by any tests? Is it even possible to fetch this by any kind of test?

@matgnt
Copy link

matgnt commented Apr 5, 2023

@tuncaytunc-zf do you know, why this wasn't catched by any tests? Is it even possible to fetch this by any kind of test?

Fully agree. I also wanted to create a few black box tests for the image (which can be only additional tests to what you mentioned) and what limits me is that with the image you have to use azure vault and I can not mock it because of hardcoded domain name in AzureVault.java

.vaultUrl("https://" + keyVaultName + ".vault.azure.net")

If no security reason is against this, I would propose to make this configurable.

@tuncaytunc-zf
Copy link
Contributor Author

@tuncaytunc-zf do you know, why this wasn't catched by any tests? Is it even possible to fetch this by any kind of test?

Probably this bug was made by Gradle migration. Unfortunately our tests don't cover so much, in our business-test we just test one image "edc-controlplane-postgresql-hashicorp-vault". This is not the only bug we couldn't catch, there were many others too which could be caught when we would have more tests in place.

@wolf4ood
Copy link
Contributor

wolf4ood commented Apr 5, 2023

Thanks why I raised this one

#148

The business tests runs only on a specific configuration, If we could leverage the Helm testing framework for checking the other charts variants probably would be a good idea

@tuncaytunc-zf
Copy link
Contributor Author

Of course we need more tests but starting such a fundamental discussion on such a minor bugfix makes no sense for me.
We should have had such a discussion before we made a lot of fundamental changes in our code where we fixed them afterwards and still fixing, this is not the only one we have fixed. It seems for me we didn't test our fundamental changes so much. I think we could spend more time on making our products more stable instead of refactoring running/stable code were we introduce a lot of new bugs, since we really don't have so much tests. Instead of implementing new tests we have already started to refactor the existing ones 😃

Copy link

@stefan-ettl stefan-ettl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi all, let's focus on the fix first, as Tuncay said.

@Siegfriedk Siegfriedk merged commit c95c982 into eclipse-tractusx:develop Apr 5, 2023
paullatzelsperger pushed a commit to paullatzelsperger/tractusx-edc that referenced this pull request May 4, 2023
paullatzelsperger pushed a commit that referenced this pull request May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants