-
Notifications
You must be signed in to change notification settings - Fork 75
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 #760 by making administrator_login & administrator_password optional for connection details #761
Fix #760 by making administrator_login & administrator_password optional for connection details #761
Conversation
…tor_password optional for connection details Signed-off-by: Nikolas Philips <nikolas.philips@gmail.com>
/test-examples="examples/dbforpostgresql/v1beta2/flexibleserver.yaml" |
@jeanduplessis Thanks a lot for letting the tests run. I see the e2e tests failed because of:
resp.
I could be mistaken, but this does not seem to be related to my changes at all. Is there a case where this e2e test for flexible server v1beta2 has run through? I surely could invest some time to fix the e2e tests - but first wanted to have confirmation from someone else that this is more of a "general e2e test"-issue than related to my code changes. |
Hey @niiku, many thanks for your effort and interest here, we appreciate it.
Yes, this is not related to the change you made, the e2e test of the current version of the resource seems to be unverified. Dependent resources need to be checked and selectors need to be adjusted. |
@turkenf Thanks a lot for your answer! I could have a look at the e2e tests. As far as I understood the code base, there aren't that many unit tests done for config.go - so I wanted to get some input how to best "bake in" the use case I added in form of tests. Might you guide me if you rather would like to see unit tests (as this might be well testable on this level) and/or have an e2e for this case as well? |
@jeanduplessis @turkenf I think i might have fixed the e2e as simply the privat dns name wasn't set correctly. Could you let them run again? |
Signed-off-by: Nikolas Philips <nikolas.philips@baloise.com>
b8e157d
to
38e90ec
Compare
/test-examples="examples/dbforpostgresql/v1beta2/flexibleserver.yaml" |
Signed-off-by: Nikolas Philips <nikolas.philips@baloise.com>
Hey @niiku, I have manually checked the resource's
|
…allow distribution / increase default postgresql version as 16 is now latest Signed-off-by: Nikolas Philips <nikolas.philips@baloise.com>
/test-examples="examples/dbforpostgresql/v1beta2/flexibleserver.yaml" |
I added the following changes:
I would not recommend to increase the timeout - as this setup should be provisioned in like ~10min - so 30min as timeout is sufficient - if it takes longer it just delays the time until we get feedback from the e2e tests. Could you run the e2e tests again? |
Signed-off-by: Nikolas Philips <nikolas.philips@baloise.com>
/test-examples="examples/dbforpostgresql/v1beta2/flexibleserver.yaml" |
1 similar comment
/test-examples="examples/dbforpostgresql/v1beta2/flexibleserver.yaml" |
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.
Many thanks for your effort @niiku 🙌
Description of your changes
As described in #760 disabling password auth (only enabling azure ad) results in a panic as no credential details are available. Within this PR checks are introduced which verify the existence of username/password and only then add it to the credential details.
As general "inspiration" I oriented the change on how it's done for MSSQL: https://github.com/crossplane-contrib/provider-upjet-azure/blob/main/config/sql/config.go#L39
Fixes #760
I have:
make reviewable
to ensure this PR is ready for review. <- I somehow didn't get that command running. But I doubt it's because of my code changes..backport release-x.y
labels to auto-backport this PR if necessary. <- Not too sure. I need input from the maintainers if it makes sense to backport?How has this code been tested
I run the fixed provider within my cluster with the example MR as described in the bug report. With the applied changes, the panic is not appearing and the .status of the FlexibleServer MR is updated when creating with disabled password auth.
A successful uptest run of the resource: https://github.com/crossplane-contrib/provider-upjet-azure/actions/runs/9483541667