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: Provide more graceful handling for APIM default cleanup #225

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

tomkerkhove
Copy link
Contributor

@tomkerkhove tomkerkhove commented Oct 4, 2021

Signed-off-by: Tom Kerkhove [email protected]

Relates to #216

@netlify
Copy link

netlify bot commented Oct 4, 2021

✔️ Deploy Preview for arcus-scripting canceled.

🔨 Explore the source changes: 6b7004b

🔍 Inspect the deploy log: https://app.netlify.com/sites/arcus-scripting/deploys/619373a5a492f50007e949b3

@stijnmoreels
Copy link
Member

Should not close this issue, right?

@tomkerkhove
Copy link
Contributor Author

Yes and no, it will no longer fail if it's not there (based on my basic PowerShell knowledge)

@tomkerkhove
Copy link
Contributor Author

A unit test failed but I'm new to Pester - Where do I find them and how do I run them?

@stijnmoreels
Copy link
Member

stijnmoreels commented Oct 5, 2021

Yes and no, it will no longer fail if it's not there (based on my basic PowerShell knowledge)

Hmm, smells like a workaround then, as there's no change in actual operation calls.

@stijnmoreels
Copy link
Member

A unit test failed but I'm new to Pester - Where do I find them and how do I run them?

Invoke Pester ./src/Arcus.Scripting.Tests.Unit/Arcus.Scripting.ApiManagement.tests.ps1

Make sure that the .psd1 file of the APIM is having a valid version in the ModuleVersion (instead of the token, like 0.1), otherwise Pester will not be able to read the PS module.
Also: https://github.com/arcus-azure/arcus.scripting/blob/master/CONTRIBUTING.md#how-to-add-tests-for-a-new-script

@tomkerkhove
Copy link
Contributor Author

tomkerkhove commented Oct 5, 2021

Changed it to "Relates to" but we need to fix this

@tomkerkhove
Copy link
Contributor Author

A unit test failed but I'm new to Pester - Where do I find them and how do I run them?

Invoke Pester ./src/Arcus.Scripting.Tests.Unit/Arcus.Scripting.ApiManagement.tests.ps1

Make sure that the .psd1 file of the APIM is having a valid version in the ModuleVersion (instead of the token, like 0.1), otherwise Pester will not be able to read the PS module. Also: https://github.com/arcus-azure/arcus.scripting/blob/master/CONTRIBUTING.md#how-to-add-tests-for-a-new-script

Looking at the tests it's supposed to return a bool but in the latest version it's only returning an empty object which is the reason of this behavior I think.

Must be a breaking change that slipped in.

@stijnmoreels
Copy link
Member

A unit test failed but I'm new to Pester - Where do I find them and how do I run them?

Invoke Pester ./src/Arcus.Scripting.Tests.Unit/Arcus.Scripting.ApiManagement.tests.ps1
Make sure that the .psd1 file of the APIM is having a valid version in the ModuleVersion (instead of the token, like 0.1), otherwise Pester will not be able to read the PS module. Also: https://github.com/arcus-azure/arcus.scripting/blob/master/CONTRIBUTING.md#how-to-add-tests-for-a-new-script

Looking at the tests it's supposed to return a bool but in the latest version it's only returning an empty object which is the reason of this behavior I think.

Must be a breaking change that slipped in.

I don't see any script functions that return booleans, though. If you refering the the stubbed-out functions, those are Azure-specific functions from a dependent module. The tests are failing here because the stubbed-out functions are returning something and the new proposed functionality here assumes that returning something means something's broken.

@tomkerkhove
Copy link
Contributor Author

I know but that's the new reality, at least in the versions that I have 🤷‍♂️

@stijnmoreels
Copy link
Member

Ok, so we may need to change the tests here.

@tomkerkhove
Copy link
Contributor Author

Unfortunately I won't have the bandwidth to change this so if somebody else could pick this up that would be great.

We also need to check if this is introduced in a given version or not.

@tomkerkhove
Copy link
Contributor Author

@stijnmoreels As I won't have time to complete this, are you willing to take this over or should I abandon the PR?

@stijnmoreels
Copy link
Member

@stijnmoreels As I won't have time to complete this, are you willing to take this over or should I abandon the PR?

Ok, I'll take this up when I got the time (😓).

@tomkerkhove
Copy link
Contributor Author

It's not urgent and sorry!

@stijnmoreels stijnmoreels merged commit 62568cb into arcus-azure:master Nov 16, 2021
@tomkerkhove tomkerkhove deleted the apim-defaults branch November 16, 2021 09:29
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.

2 participants