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 CI tests for different target connection configs #75

Merged
merged 17 commits into from
Jan 5, 2021
Merged

Conversation

dataders
Copy link
Collaborator

@dataders dataders commented Jan 5, 2021

for #65. basically now we have 3 different target configs to test against:

  • SQL Server
    • when Encrypt and TrustServerCertificate aren't specified
  • when Encrypt and TrustServerCertificate are specified
  • Azure SQL using SQL Password

Connection types still lacking coverage are:

  • windows_login=True because it would require a Windows VM, but we can open a new PR for that in the future.
  • Active Directory Password because of below. IMHO, not an issue since we'll be supported AAD login via the az cli shortly anyway
    Could not discover endpoint for username/password authentication. Check your ADFS settings.
    It should support username/password authentication for WS-Trust 1.3 or WS-Trust 2005. (System.Data)
    
  • ActiveDirectoryServicePrincipal for the same reason as above.

@Mikael any must have connection type coverage? If not, I'd say let's merge this so we can:

@dataders dataders requested a review from mikaelene January 5, 2021 05:48
@dataders dataders marked this pull request as ready for review January 5, 2021 05:49
encrypt: yes
trust_cert: yes
azuresql_sqlcred: &azuresql
<<: *sqlserver
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, is this a way to extend de above mentioned profiles, the ones with &<some name>?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's right!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they're called YAML anchors, and I'm on the fence about them legibility-wise. Seems pretty easy to go overboard on. how would you rate the readability of this YAML file, @JCZuurmond?
https://support.atlassian.com/bitbucket-cloud/docs/yaml-anchors/

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the anchors, indeed you should not use it too much, but it narrows what your focusing on within each segment. For example with sqlserver_local_encrypt it is clear that encrypt: yes and trust_cert: yes is what should be tested - it's like a fixture in testing

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

I like it!

encrypt: yes
trust_cert: yes
azuresql_sqlcred: &azuresql
<<: *sqlserver
Copy link
Contributor

Choose a reason for hiding this comment

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

that's right!

@mikaelene
Copy link
Collaborator

Great Work!

@mikaelene mikaelene merged commit a7f0a23 into master Jan 5, 2021
@mikaelene mikaelene deleted the cnxn_ci branch January 20, 2021 08:42
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.

4 participants