How to submit a PR for multiple but related changes

[pshamus]

I have refactored SqlDatabaseRole to function like SqlServerRole, where roles and their members can be managed. This will potentially address issues #845, #847, #1252, and #1339. The new functionality removes the auto-adding of SQL logins as database users. However, I have broken that out into a separate resource (SqlDatabaseUser) to allow more fine-grained control over the mappings (e.g. re-mapping of orphaned logins, different login and user names). Given that the new changes need to go together, what is the right process for a PR? Should I include both resource changes in the same PR?

In addition, I don't yet have unit tests for SqlDatabaseUser. Is that required for submission?

[johlju]

Really awesome work! 😃 👍 To make it easier for me as a maintainer I suggest sending in the new resource in a separate PR, and the other changes in a second PR. You can send in both PR's at the same time.
But if both PR's need to share new helper functions then I'm okay with having all changes in the same PR.

[johlju]

In addition, I don't yet have unit tests for SqlDatabaseUser. Is that required for submission?

Unit test is required for me to be able to merge the changes. All changes must be tested.
Also where integration tests has been added, the integration tests should be updated, or if there are no integration tests I would love for them to get added. I won't enforce integration tests if there are none today.

[pshamus]

Perfect, thanks for the reply. I just submitted a PR for SqlDatabaseRole and will work on the unit tests for SqlDatabaseUser.

[ykuijs]

With v13.0 the SqlDatabaseRole PR has been merged, but the SqlDatabaseUser PR isn't submitted yet. How should I add users to a database without this new resource?

[johlju]

Oh, I lost track of that - we shouldn’t have released SqlServerDsc without that new resource. :/

@pshamus how far along are you with the new SqlDatabaseUser resource? If you don’t have time to work on it, then it’s okay, it if you have something finished, maybe I can help continue the work?

[pshamus]

I don’t have unit tests yet, but the code works, as I am using it already. Can I check it in and get some help with the tests?

[johlju]

That is great! Please send in a PR with what you got and I will gladly help out getting tests done. 😊

[pshamus]

Sounds good, will do. Should I include changes to README.md and CHANGELOG.md?

[johlju]

Yes please do if you can put in the time. 🙂

[johlju]

@ykuijs SqlDatabaseUser resource merged. Not sure we can get it released until next release window though.

[ykuijs]

No problem, as a workaround I am using v12.5 now :-)