Feature - redesign library with first cycle of refactoring #27

stijnmoreels · 2020-05-25T09:08:17Z

Separate logic app "single instance" and "many instances" functionality

Functionality on a single logic app is placed in the LogicAppClient while the polling is moved to the LogicAppsProvider.

Introduce 'temporary' functionality

This makes sure the setup/teardown of logic app operations are more managed/controlled.

using (logicApp.TemporaryEnableStaticResult("<action-name>"))
{
    // On dispose: disable static result.
}

Building polling function

Reworked the different polling functions available in a more streamlined/fluent design.

Task<IEnumerable<LogicAppRun>> pollingTask = LogicAppsProvider
    .LocatedAt(ResourceGroup, LogicAppName, Authentication, Logger)
    .WithTrackedProperty(trackedPropertyName, trackedPropertyValue)
    .WithTimeout(timeout)
    .PollForLogicAppRunsAsync(numberOfRuns);

Relates to #3

stijnmoreels · 2020-05-25T09:08:52Z

First proposal, @mbraekman , for the redesign of the library.
All is just proposal, all can be changed 😉 .

Feel free to give some feedback so I can make this better + bigger changes we can place in separate issues of course instead of pushing to this PR.

src/Invictus.Testing/Invictus.Testing.csproj

src/Invictus.Testing/Model/LogicApp.cs

src/Invictus.Testing.Tests.Integration/LogicAppClientTests.cs

mbraekman

Just some minor questions or request to add some additional tests.
You're doing a good job!

src/Invictus.Testing.Tests.Integration/LogicAppClientTests.cs

src/Invictus.Testing/LogicAppClient.cs

tomkerkhove

This looks like a great API from a consumer perspective, well done 🎉

I've added a bunch of comments and suggestions but mainly nitpicking or testing requirements.

Next to that, I think we can introduce a bunch of unit tests for:

Verifing our converter is working
Ensure parameter validation works for LogicAuthentication, LogicAppTriggerNotFoundException & LogicAppClient

src/Invictus.Testing.Tests.Integration/LogicAppClientTests.cs

src/Invictus.Testing/LogicAppsProvider.cs

src/Invictus.Testing/LogicAppClient.cs

src/Invictus.Testing/Converter.cs

src/Invictus.Testing/LogicAppsProvider.cs

… poll

src/Invictus.Testing/LogicAppException.cs

tomkerkhove · 2020-06-11T09:46:26Z

src/Invictus.Testing/LogicAppException.cs

+        /// <param name="logicAppName">The name of the logic app resource running in Azure.</param>
+        /// <param name="resourceGroup">The resource group where the logic app is located.</param>
+        /// <param name="subscriptionId">The ID that identifies the subscription on Azure.</param>
+        public LogicAppException(


We should always order from least to most specific meaning - subscription, resource group, logic app name, message

src/Invictus.Testing/LogicAppException.cs

src/Invictus.Testing.Tests.Integration/LogicAppNotUpdatedExceptionTests.cs

tomkerkhove · 2020-06-11T09:53:01Z

src/Invictus.Testing/LogicAppsProvider.cs

+        {
+            Guard.NotLessThanOrEqualTo(minimumNumberOfItems, 0, nameof(minimumNumberOfItems));
+
+            string amount = minimumNumberOfItems == 1 ? "any" : minimumNumberOfItems.ToString();


Frankly, this still feels off. Both our methods relying on this provide 1 so we'll always use any which is not correct if you ask me.

I'd say that, if you want all runs, you don't provide this property and poll for a given amount of time or so which uses another flow than pulling for one; no? They are different but yet use same exact approach/parameters.

Isn't that a problem, since there could be an continuously amount of runs? And we're never sure if we have all runs. I think that's the reason why this was in the initial design.

We could use a max timeout but if that's how it worked before then that's it 🤷‍♂️

The polling-mechanisms should have a max timeout, so either it returns the given amount of runs or it returns what it found after the max interval expired.
But that should've implemented, so should be OK.

Ok, then I guess I have to add two different approaches on this.

Ok, @mbraekman , this is now changed to this two-system approach. Can you approve/reject on this new changes? Thanks.

src/Invictus.Testing/LogicAppConverter.cs

tomkerkhove · 2020-06-11T09:55:04Z

src/Invictus.Testing/LogicAppClient.cs

+        /// Runs the current logic app resource by searching for triggers on the logic app.
+        /// </summary>
+        /// <exception cref="LogicAppTriggerNotFoundException">When no trigger can be found on the logic app.</exception>
+        public async Task RunAsync()


It starts a Logic App without you having to worry about the trigger name; that's what I think based on the implementation.

Is that correct @mbraekman ?

tomkerkhove · 2020-06-11T09:55:42Z

src/Invictus.Testing/LogicAppClient.cs

+            Guard.NotNullOrEmpty(triggerName, nameof(triggerName));
+
+            _logger.LogTrace("Run the workflow trigger of logic app '{LogicAppName}' in resource group '{ResourceGroup}'", _logicAppName, _resourceGroup);
+            await _logicManagementClient.WorkflowTriggers.RunAsync(_resourceGroup, _logicAppName, triggerName);


Then we should log that for sure :)

Sounds like a test going wrong, no? Can you help here @mbraekman ?

tomkerkhove

Lost track of all the feedback but think most of it is said so giving a 👍 once you think everything is processed @stijnmoreels

src/Invictus.Testing/LogicAppClient.cs

Co-authored-by: Maxim Braekman <[email protected]>

Feature - redesign library with first cycle of refactoring

a8ae60c

stijnmoreels requested a review from tomkerkhove as a code owner May 25, 2020 09:08

stijnmoreels assigned mbraekman May 25, 2020

stijnmoreels added 2 commits May 25, 2020 11:10

pr-fix: finish renaming

844c398

pr-fix: use string as output result

92ea233

stijnmoreels commented May 25, 2020

View reviewed changes

src/Invictus.Testing/Invictus.Testing.csproj Show resolved Hide resolved

stijnmoreels commented May 25, 2020

View reviewed changes

src/Invictus.Testing/Model/LogicApp.cs Show resolved Hide resolved

stijnmoreels linked an issue May 26, 2020 that may be closed by this pull request

Redesign library APIs #3

Closed

mbraekman reviewed Jun 4, 2020

View reviewed changes

src/Invictus.Testing.Tests.Integration/LogicAppClientTests.cs Outdated Show resolved Hide resolved

mbraekman suggested changes Jun 4, 2020

View reviewed changes

src/Invictus.Testing.Tests.Integration/LogicAppClientTests.cs Show resolved Hide resolved

src/Invictus.Testing/LogicAppClient.cs Show resolved Hide resolved

stijnmoreels added 2 commits June 5, 2020 08:35

pr-sug: test temp enabling logic app

12b9757

pr-sug: split 'GetLogicAppTriggerUrlAsync' to better alternative

7636afc

tomkerkhove reviewed Jun 8, 2020

View reviewed changes

stijnmoreels added 16 commits June 9, 2020 08:13

pr-sug: use Arcus guidelines for test's names

f7e159c

pr-sug: use date time offset

d62a856

pr-sug: use single or default

8d8227c

pr-sug: remove magic number of runs

b91a0fc

pr-sug: increase timeout to 40s

19c7252

pr-sug: add explicit test cases for logic apps provider

761f560

pr-sug: add extra information to the trigger exception

afa75e6

pr-sug: use '...Async' suffix on all asynchronous methods

f33684d

pr-sug: update without booleans in result type

3944b54

pr-sug: add arg checks

947d947

pr-sug: add test for temp success static result

f406630

pr-sug: update with any derived exception assertion

b932c9a

pr-sug: using datetime offset

e3e0bc4

pr-sug: rename to 'LogicAppConverter'

233217b

pr-sug: use private disabling func.

11484af

pr-sug: move tenant ID before subscription ID in logic app auth.

68ec399

tomkerkhove reviewed Jun 10, 2020

View reviewed changes

src/Invictus.Testing/LogicAppsProvider.cs Outdated Show resolved Hide resolved

src/Invictus.Testing/LogicAppsProvider.cs Outdated Show resolved Hide resolved

src/Invictus.Testing/LogicAppsProvider.cs Outdated Show resolved Hide resolved

stijnmoreels linked an issue Jun 11, 2020 that may be closed by this pull request

Revisit logic app custom models #35

Closed

pr-sug: rename to include 'minimum...' in number of logic app runs to…

6b25c73

… poll

tomkerkhove reviewed Jun 11, 2020

View reviewed changes

stijnmoreels added 4 commits June 12, 2020 07:53

pr-sug: add tests for creating/assigning info in custom exceptions

bcb3e9e

pr-sug: add tests for logic app converter

d106d4b

pr-fix: update with correct double assertion of possible 'null'

c104e3e

pr-sug: update exception signature with message as least specific

d56e4d1

stijnmoreels mentioned this pull request Jun 12, 2020

Provide Logic App + ARM used to test "TemporaryUpdateAsync" and "RunAsync" #31

Closed

2 tasks

pr-sug: update exception signature with message as least specific

efff563

tomkerkhove approved these changes Jun 12, 2020

View reviewed changes

stijnmoreels added 3 commits June 15, 2020 08:50

pr-sug: use diff setup for timeout/number of items

144705d

pr-sug: testing reconfigure

2676472

pr-sug: update simpler assertion

2e43b63

mbraekman approved these changes Jun 15, 2020

View reviewed changes

src/Invictus.Testing/LogicAppClient.cs Outdated Show resolved Hide resolved

src/Invictus.Testing/LogicAppClient.cs Outdated Show resolved Hide resolved

stijnmoreels and others added 2 commits June 15, 2020 12:49

Update src/Invictus.Testing/LogicAppClient.cs

be6b744

Co-authored-by: Maxim Braekman <[email protected]>

Update src/Invictus.Testing/LogicAppClient.cs

496c733

Co-authored-by: Maxim Braekman <[email protected]>

stijnmoreels merged commit f4f624b into invictus-integration:master Jun 15, 2020

stijnmoreels deleted the feature/redesign-library branch August 30, 2021 08:27

Feature - redesign library with first cycle of refactoring #27

Feature - redesign library with first cycle of refactoring #27

Uh oh!

Conversation

stijnmoreels commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Separate logic app "single instance" and "many instances" functionality

Introduce 'temporary' functionality

Building polling function

Uh oh!

stijnmoreels commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbraekman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomkerkhove left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomkerkhove left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stijnmoreels commented May 25, 2020 •

edited

Loading

stijnmoreels commented May 25, 2020 •

edited

Loading