-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: Integration Account - Provide script to upload schemas. #178
chore: Integration Account - Provide script to upload schemas. #178
Conversation
…Related to arcus-azure#160 in Arcus.Scripting
…azure#160 on Arcus.Scripting
The # Version number of this module.
ModuleVersion = '0.1' But: # Version number of this module.
ModuleVersion = '#{Package.Version}#'
|
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.
Maybe some unit tests can test those special-case guards for the SchemaFilePath
and the SchemasFolder
, and possible other guards.
But otherwise, awesome!
src/Arcus.Scripting.IntegrationAccount/Arcus.Scripting.IntegrationAccount.psm1
Outdated
Show resolved
Hide resolved
src/Arcus.Scripting.IntegrationAccount/Arcus.Scripting.IntegrationAccount.psm1
Show resolved
Hide resolved
src/Arcus.Scripting.IntegrationAccount/Scripts/Set-AzIntegrationAccountSchemas.ps1
Outdated
Show resolved
Hide resolved
src/Arcus.Scripting.IntegrationAccount/Scripts/Set-AzIntegrationAccountSchemas.ps1
Outdated
Show resolved
Hide resolved
src/Arcus.Scripting.IntegrationAccount/Scripts/Set-AzIntegrationAccountSchemas.ps1
Outdated
Show resolved
Hide resolved
src/Arcus.Scripting.IntegrationAccount/Scripts/Set-AzIntegrationAccountSchemas.ps1
Outdated
Show resolved
Hide resolved
} | ||
else { | ||
if ($SchemasFolder -ne '' -and $SchemaFilePath -eq '') { | ||
foreach ($schema in Get-ChildItem("$schemasFolder") -File) { |
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.
Just curious. Placing the string folder into another string, is this some extra-guard for string formatting or some kind?
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.
No, more because I made a mistake here. Tested this with a hard-coded value first but didn't replace the quotes surrounding the hard-coded value 😅
src/Arcus.Scripting.Tests.Integration/Arcus.Scripting.IntegrationAccount.tests.ps1
Outdated
Show resolved
Hide resolved
Import-Module Az.KeyVault | ||
Import-Module -Name $PSScriptRoot\..\Arcus.Scripting.IntegrationAccount -ErrorAction Stop | ||
|
||
InModuleScope Arcus.Scripting.IntegrationAccount { |
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.
SO GREAT! Directly integration tests from the start. Great. 👍
I was working on a separate script so we can re-use some of these Azure connecting functionality.
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.
Great, we can perhaps refactor this is the same branch where you're creating the reusable connectivity scripts.
If that's okay with you of course.
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.
Yeah, of course! Let me know if you have time here, but I can do a follow-up PR too.
src/Arcus.Scripting.Tests.Integration/Arcus.Scripting.IntegrationAccount.tests.ps1
Show resolved
Hide resolved
Woops, missed that one 😅 |
…com/mbraekman/arcus.scripting into feature/integration-account-schemas
😄 I also change this locally to test it locally. I'll have a look in the future if we can maybe change this a bit so it's easier to test locally. |
@stijnmoreels : I've added some unit tests, if you would so kind to review this, please? |
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.
LGTM! Nice, clean but thorough tests. 💯
This PR closes #160
In order to align with other scripts, the following parameters mentioned in the issue have been renamed:
The following parameters have been added:
Module:
A new module has been created for this:
Arcus.Scripting.IntegrationAccount
.As this will be extended by all other scripts required for the open issues on this topic.
Documentation:
A new page (Integration Account) has been added in the preview folder + update on the index.md to refer to the new page.
Once this PR has been reviewed/approved the other issues will be taken care of and set up similar to this.